Fixed: [Issue 201] Import validation cannot handle circular
dependencies.
diff --git a/com.google.eclipse.protobuf.integration.test/src/com/google/eclipse/protobuf/validation/ImportValidator_checkNonProto2Imports_Test.java b/com.google.eclipse.protobuf.integration.test/src/com/google/eclipse/protobuf/validation/ImportValidator_checkNonProto2Imports_Test.java
new file mode 100644
index 0000000..5b9a99b
--- /dev/null
+++ b/com.google.eclipse.protobuf.integration.test/src/com/google/eclipse/protobuf/validation/ImportValidator_checkNonProto2Imports_Test.java
@@ -0,0 +1,77 @@
+/*
+ * Copyright (c) 2012 Google Inc.
+ *
+ * All rights reserved. This program and the accompanying materials are made available under the terms of the Eclipse
+ * Public License v1.0 which accompanies this distribution, and is available at
+ *
+ * http://www.eclipse.org/legal/epl-v10.html
+ */
+package com.google.eclipse.protobuf.validation;
+
+import static com.google.eclipse.protobuf.junit.core.IntegrationTestModule.integrationTestModule;
+import static com.google.eclipse.protobuf.junit.core.XtextRule.overrideRuntimeModuleWith;
+import static org.mockito.Mockito.*;
+
+import org.eclipse.xtext.validation.ValidationMessageAcceptor;
+import org.junit.*;
+
+import com.google.eclipse.protobuf.junit.core.XtextRule;
+import com.google.eclipse.protobuf.protobuf.Protobuf;
+import com.google.inject.Inject;
+
+/**
+ * Tests for <code>{@link ImportValidator#checkNonProto2Imports(Protobuf)}</code>
+ *
+ * @author alruiz@google.com (Alex Ruiz)
+ */
+public class ImportValidator_checkNonProto2Imports_Test {
+ @Rule public XtextRule xtext = overrideRuntimeModuleWith(integrationTestModule());
+
+ @Inject private ImportValidator validator;
+ private ValidationMessageAcceptor messageAcceptor;
+
+ @Before public void setUp() {
+ messageAcceptor = mock(ValidationMessageAcceptor.class);
+ validator.setMessageAcceptor(messageAcceptor);
+ }
+
+ // // Create file C.proto
+ //
+ // syntax = 'proto2';
+
+ // // Create file B.proto
+ //
+ // syntax = 'proto2';
+ //
+ // import "C.proto";
+
+ // syntax = "proto2";
+ //
+ // import "B.proto";
+ // import "C.proto";
+ @Test public void should_not_add_warnings_if_imported_files_are_proto2() {
+ validator.checkNonProto2Imports(xtext.root());
+ verifyNoMoreInteractions(messageAcceptor);
+ }
+
+ // // Create file C.proto
+ //
+ // syntax = 'proto2';
+ //
+ // import "B.proto";
+
+ // // Create file B.proto
+ //
+ // syntax = 'proto2';
+ //
+ // import "C.proto";
+
+ // syntax = "proto2";
+ //
+ // import "B.proto";
+ // import "C.proto";
+ @Test public void should_not_add_warnings_if_imported_files_are_proto2_even_with_circular_dependencies() {
+ validator.checkNonProto2Imports(xtext.root());
+ verifyNoMoreInteractions(messageAcceptor);
+ }
+}
diff --git a/com.google.eclipse.protobuf.integration.test/src/com/google/eclipse/protobuf/validation/ImportValidator_checkNonProto2Imports_withNonProto2Imports_Tests.java b/com.google.eclipse.protobuf.integration.test/src/com/google/eclipse/protobuf/validation/ImportValidator_checkNonProto2Imports_withNonProto2Imports_Tests.java
new file mode 100644
index 0000000..d8f8aa9
--- /dev/null
+++ b/com.google.eclipse.protobuf.integration.test/src/com/google/eclipse/protobuf/validation/ImportValidator_checkNonProto2Imports_withNonProto2Imports_Tests.java
@@ -0,0 +1,118 @@
+/*
+ * Copyright (c) 2012 Google Inc.
+ *
+ * All rights reserved. This program and the accompanying materials are made available under the terms of the Eclipse
+ * Public License v1.0 which accompanies this distribution, and is available at
+ *
+ * http://www.eclipse.org/legal/epl-v10.html
+ */
+package com.google.eclipse.protobuf.validation;
+
+import static com.google.eclipse.protobuf.junit.core.IntegrationTestModule.integrationTestModule;
+import static com.google.eclipse.protobuf.junit.core.XtextRule.overrideRuntimeModuleWith;
+import static com.google.eclipse.protobuf.protobuf.ProtobufPackage.Literals.IMPORT__IMPORT_URI;
+import static com.google.eclipse.protobuf.validation.Messages.importingNonProto2;
+import static org.eclipse.xtext.validation.ValidationMessageAcceptor.INSIGNIFICANT_INDEX;
+import static org.mockito.Mockito.*;
+
+import java.util.List;
+
+import org.eclipse.emf.common.util.URI;
+import org.eclipse.xtext.validation.ValidationMessageAcceptor;
+import org.junit.*;
+
+import com.google.eclipse.protobuf.junit.core.*;
+import com.google.eclipse.protobuf.model.util.Protobufs;
+import com.google.eclipse.protobuf.protobuf.*;
+import com.google.inject.*;
+
+/**
+ * Tests for <code>{@link ImportValidator#checkNonProto2Imports(Protobuf)}</code>
+ *
+ * @author alruiz@google.com (Alex Ruiz)
+ */
+public class ImportValidator_checkNonProto2Imports_withNonProto2Imports_Tests {
+ @Rule public XtextRule xtext = overrideRuntimeModuleWith(integrationTestModule(), new TestModule());
+
+ @Inject private ProtobufsStub protobufs;
+ @Inject private ImportValidator validator;
+
+ private ValidationMessageAcceptor messageAcceptor;
+
+ @Before public void setUp() {
+ messageAcceptor = mock(ValidationMessageAcceptor.class);
+ validator.setMessageAcceptor(messageAcceptor);
+ }
+
+ // // Create file C.proto
+ //
+ // syntax = 'proto2';
+
+ // // Create file B.proto
+ //
+ // syntax = 'proto2';
+
+ // syntax = "proto2";
+ //
+ // import "B.proto";
+ // import "C.proto";
+ @Test public void should_add_warning_if_import_if_import_refers_directly_to_non_proto2() {
+ protobufs.nonProto2FileName = "B.proto";
+ validator.checkNonProto2Imports(xtext.root());
+ Import importWithNonProto2File = findImportReferreringToFile(protobufs.nonProto2FileName);
+ verifyThatImportingNonProto2FileCreatedWarning(importWithNonProto2File);
+ }
+
+ // // Create file C.proto
+ //
+ // syntax = 'proto2';
+
+ // // Create file B.proto
+ //
+ // syntax = 'proto2';
+ //
+ // import "C.proto";
+
+ // syntax = "proto2";
+ //
+ // import "B.proto";
+ @Test public void should_add_warning_if_import_if_import_refers_indirectly_to_non_proto2() {
+ protobufs.nonProto2FileName = "C.proto";
+ validator.checkNonProto2Imports(xtext.root());
+ Import importWithNonProto2File = findImportReferreringToFile("B.proto");
+ verifyThatImportingNonProto2FileCreatedWarning(importWithNonProto2File);
+ }
+
+ private Import findImportReferreringToFile(String fileName) {
+ List<Import> imports = protobufs.importsIn(xtext.root());
+ for (Import anImport : imports) {
+ if (anImport.getImportURI().endsWith(fileName)) {
+ return anImport;
+ }
+ }
+ return null;
+ }
+
+ private void verifyThatImportingNonProto2FileCreatedWarning(Import anImport) {
+ verify(messageAcceptor).acceptWarning(importingNonProto2, anImport, IMPORT__IMPORT_URI, INSIGNIFICANT_INDEX, null,
+ new String[0]);
+ }
+
+ private static class TestModule extends AbstractTestModule {
+ @Override protected void configure() {
+ binder().bind(Protobufs.class).to(ProtobufsStub.class);
+ }
+ }
+
+ @Singleton private static class ProtobufsStub extends Protobufs {
+ String nonProto2FileName;
+
+ @Override public boolean isProto2(Protobuf protobuf) {
+ URI resourceUri = protobuf.eResource().getURI();
+ if (resourceUri.toString().endsWith(nonProto2FileName)) {
+ return false;
+ }
+ return super.isProto2(protobuf);
+ }
+ }
+}
diff --git a/com.google.eclipse.protobuf.test/src/com/google/eclipse/protobuf/junit/stubs/resources/ResourceStub.java b/com.google.eclipse.protobuf.test/src/com/google/eclipse/protobuf/junit/stubs/resources/ResourceStub.java
deleted file mode 100644
index f69bd43..0000000
--- a/com.google.eclipse.protobuf.test/src/com/google/eclipse/protobuf/junit/stubs/resources/ResourceStub.java
+++ /dev/null
@@ -1,163 +0,0 @@
-/*
- * Copyright (c) 2011 Google Inc.
- *
- * All rights reserved. This program and the accompanying materials are made available under the terms of the Eclipse
- * Public License v1.0 which accompanies this distribution, and is available at
- *
- * http://www.eclipse.org/legal/epl-v10.html
- */
-package com.google.eclipse.protobuf.junit.stubs.resources;
-
-import static org.eclipse.emf.common.util.URI.createURI;
-
-import java.io.*;
-import java.util.Map;
-
-import org.eclipse.emf.common.notify.*;
-import org.eclipse.emf.common.util.*;
-import org.eclipse.emf.ecore.EObject;
-import org.eclipse.emf.ecore.resource.*;
-
-/**
- * @author alruiz@google.com (Alex Ruiz)
- */
-public class ResourceStub implements Resource {
- private URI uri;
-
- public ResourceStub() {}
-
- public ResourceStub(String uri) {
- setURI(createURI(uri));
- }
-
- /** {@inheritDoc} */
- @Override public EList<Adapter> eAdapters() {
- throw new UnsupportedOperationException();
- }
-
- /** {@inheritDoc} */
- @Override public boolean eDeliver() {
- throw new UnsupportedOperationException();
- }
-
- /** {@inheritDoc} */
- @Override public void eSetDeliver(boolean deliver) {
- throw new UnsupportedOperationException();
- }
-
- /** {@inheritDoc} */
- @Override public void eNotify(Notification notification) {
- throw new UnsupportedOperationException();
- }
-
- /** {@inheritDoc} */
- @Override public ResourceSet getResourceSet() {
- throw new UnsupportedOperationException();
- }
-
- /** {@inheritDoc} */
- @Override public URI getURI() {
- return uri;
- }
-
- /** {@inheritDoc} */
- @Override public void setURI(URI uri) {
- this.uri = uri;
- }
-
- /** {@inheritDoc} */
- @Override public long getTimeStamp() {
- throw new UnsupportedOperationException();
- }
-
- /** {@inheritDoc} */
- @Override public void setTimeStamp(long timeStamp) {
- throw new UnsupportedOperationException();
- }
-
- /** {@inheritDoc} */
- @Override public EList<EObject> getContents() {
- throw new UnsupportedOperationException();
- }
-
- /** {@inheritDoc} */
- @Override public TreeIterator<EObject> getAllContents() {
- throw new UnsupportedOperationException();
- }
-
- /** {@inheritDoc} */
- @Override public String getURIFragment(EObject eObject) {
- throw new UnsupportedOperationException();
- }
-
- /** {@inheritDoc} */
- @Override public EObject getEObject(String uriFragment) {
- throw new UnsupportedOperationException();
- }
-
- /** {@inheritDoc} */
- @Override public void save(Map<?, ?> options) {
- throw new UnsupportedOperationException();
- }
-
- /** {@inheritDoc} */
- @Override public void load(Map<?, ?> options) {
- throw new UnsupportedOperationException();
- }
-
- /** {@inheritDoc} */
- @Override public void save(OutputStream outputStream, Map<?, ?> options) {
- throw new UnsupportedOperationException();
- }
-
- /** {@inheritDoc} */
- @Override public void load(InputStream inputStream, Map<?, ?> options) {
- throw new UnsupportedOperationException();
- }
-
- /** {@inheritDoc} */
- @Override public boolean isTrackingModification() {
- throw new UnsupportedOperationException();
- }
-
- /** {@inheritDoc} */
- @Override public void setTrackingModification(boolean isTrackingModification) {
- throw new UnsupportedOperationException();
- }
-
- /** {@inheritDoc} */
- @Override public boolean isModified() {
- throw new UnsupportedOperationException();
- }
-
- /** {@inheritDoc} */
- @Override public void setModified(boolean isModified) {
- throw new UnsupportedOperationException();
- }
-
- /** {@inheritDoc} */
- @Override public boolean isLoaded() {
- throw new UnsupportedOperationException();
- }
-
- /** {@inheritDoc} */
- @Override public void unload() {
- throw new UnsupportedOperationException();
- }
-
- /** {@inheritDoc} */
- @Override public void delete(Map<?, ?> options) {
- throw new UnsupportedOperationException();
- }
-
- /** {@inheritDoc} */
- @Override public EList<Diagnostic> getErrors() {
- throw new UnsupportedOperationException();
- }
-
- /** {@inheritDoc} */
- @Override public EList<Diagnostic> getWarnings() {
- throw new UnsupportedOperationException();
- }
-
-}
diff --git a/com.google.eclipse.protobuf.test/src/com/google/eclipse/protobuf/validation/ProtobufJavaValidator_checkOnlyOnePackageDefinition_Test.java b/com.google.eclipse.protobuf.test/src/com/google/eclipse/protobuf/validation/ProtobufJavaValidator_checkOnlyOnePackageDefinition_Test.java
index 3d3a260..0ea1264 100644
--- a/com.google.eclipse.protobuf.test/src/com/google/eclipse/protobuf/validation/ProtobufJavaValidator_checkOnlyOnePackageDefinition_Test.java
+++ b/com.google.eclipse.protobuf.test/src/com/google/eclipse/protobuf/validation/ProtobufJavaValidator_checkOnlyOnePackageDefinition_Test.java
@@ -11,6 +11,7 @@
import static com.google.eclipse.protobuf.junit.core.UnitTestModule.unitTestModule;
import static com.google.eclipse.protobuf.junit.core.XtextRule.overrideRuntimeModuleWith;
import static com.google.eclipse.protobuf.protobuf.ProtobufPackage.Literals.PACKAGE__NAME;
+import static com.google.eclipse.protobuf.validation.Messages.multiplePackages;
import static com.google.eclipse.protobuf.validation.ProtobufJavaValidator.MORE_THAN_ONE_PACKAGE_ERROR;
import static org.eclipse.xtext.validation.ValidationMessageAcceptor.INSIGNIFICANT_INDEX;
import static org.mockito.Mockito.*;
@@ -45,8 +46,7 @@
@Test public void should_create_error_if_there_are_more_than_one_package_definitions() {
Package p = xtext.find("com.google.eclipse", Package.class);
validator.checkOnlyOnePackageDefinition(p);
- String message = "Multiple package definitions.";
- verify(messageAcceptor).acceptError(message, p, PACKAGE__NAME, INSIGNIFICANT_INDEX, MORE_THAN_ONE_PACKAGE_ERROR);
+ verify(messageAcceptor).acceptError(multiplePackages, p, PACKAGE__NAME, INSIGNIFICANT_INDEX, MORE_THAN_ONE_PACKAGE_ERROR);
}
// syntax = "proto2";
diff --git a/com.google.eclipse.protobuf/src/com/google/eclipse/protobuf/model/util/Imports.java b/com.google.eclipse.protobuf/src/com/google/eclipse/protobuf/model/util/Imports.java
index 43af49e..408118c 100644
--- a/com.google.eclipse.protobuf/src/com/google/eclipse/protobuf/model/util/Imports.java
+++ b/com.google.eclipse.protobuf/src/com/google/eclipse/protobuf/model/util/Imports.java
@@ -13,10 +13,12 @@
import static org.eclipse.xtext.util.Strings.*;
import org.eclipse.emf.common.util.URI;
+import org.eclipse.emf.ecore.resource.*;
import org.eclipse.xtext.nodemodel.INode;
import org.eclipse.xtext.scoping.impl.ImportUriResolver;
import com.google.eclipse.protobuf.protobuf.Import;
+import com.google.eclipse.protobuf.resource.ResourceSets;
import com.google.eclipse.protobuf.scoping.ProtoDescriptorProvider;
import com.google.inject.Inject;
@@ -28,6 +30,7 @@
public class Imports {
@Inject private ProtoDescriptorProvider descriptorProvider;
@Inject private INodes nodes;
+ @Inject private ResourceSets resourceSets;
@Inject private ImportUriResolver uriResolver;
/**
@@ -98,6 +101,21 @@
}
/**
+ * Returns the resource referred by the URI of the given {@code Import}.
+ * @param anImport the given {@code Import}.
+ * @return the resource referred by the URI of the given {@code Import}, or {@code null} if the URI has not been
+ * resolved.
+ */
+ public Resource importedResource(Import anImport) {
+ URI resolvedUri = resolvedUriOf(anImport);
+ if (resolvedUri != null) {
+ ResourceSet resourceSet = anImport.eResource().getResourceSet();
+ return resourceSets.findResource(resourceSet, resolvedUri);
+ }
+ return null;
+ }
+
+ /**
* Returns the resolved URI of the given {@code Import}.
* @param anImport the the given {@code Import}.
* @return the resolved URI of the given {@code Import}, or {@code null} if the URI was not successfully resolved.
diff --git a/com.google.eclipse.protobuf/src/com/google/eclipse/protobuf/validation/ImportValidator.java b/com.google.eclipse.protobuf/src/com/google/eclipse/protobuf/validation/ImportValidator.java
index 9f978ac..d852659 100644
--- a/com.google.eclipse.protobuf/src/com/google/eclipse/protobuf/validation/ImportValidator.java
+++ b/com.google.eclipse.protobuf/src/com/google/eclipse/protobuf/validation/ImportValidator.java
@@ -8,7 +8,7 @@
*/
package com.google.eclipse.protobuf.validation;
-import static com.google.common.collect.Lists.newArrayList;
+import static com.google.common.collect.Maps.newHashMap;
import static com.google.common.collect.Sets.newHashSet;
import static com.google.eclipse.protobuf.protobuf.ProtobufPackage.Literals.IMPORT__IMPORT_URI;
import static com.google.eclipse.protobuf.validation.Messages.*;
@@ -17,15 +17,13 @@
import java.util.*;
-import org.eclipse.emf.common.util.URI;
-import org.eclipse.emf.ecore.resource.*;
+import org.eclipse.emf.ecore.resource.Resource;
import org.eclipse.xtext.scoping.impl.ImportUriResolver;
import org.eclipse.xtext.util.Pair;
import org.eclipse.xtext.validation.*;
import com.google.eclipse.protobuf.model.util.*;
import com.google.eclipse.protobuf.protobuf.*;
-import com.google.eclipse.protobuf.resource.ResourceSets;
import com.google.inject.Inject;
/**
@@ -37,85 +35,75 @@
@Inject private Imports imports;
@Inject private Protobufs protobufs;
@Inject private Resources resources;
- @Inject private ResourceSets resourceSets;
@Inject private ImportUriResolver uriResolver;
@Override public void register(EValidatorRegistrar registrar) {}
/**
* Verifies that {@code Import}s in the given root only refer to "proto2" files. If non-proto2 {@code Import}s are
- * found, this validator will create warning markers for such "imports".
+ * found, this validator will create warning markers for such {@code Import}s.
* @param root the root containing the imports to check.
*/
@Check public void checkNonProto2Imports(Protobuf root) {
- warnIfNonProto2ImportsFound(root.eResource());
- }
-
- private void warnIfNonProto2ImportsFound(Resource resource) {
- Protobuf root = resources.rootOf(resource);
if (!protobufs.isProto2(root)) {
return;
}
- ResourceSet resourceSet = resource.getResourceSet();
- boolean hasNonProto2 = false;
- List<Pair<Import, Resource>> resourcesToCheck = newArrayList();
- Set<URI> checked = newHashSet();
- checked.add(resource.getURI());
+ Set<Protobuf> currentlyChecking = newHashSet(root);
+ HashMap<Protobuf, IsProto2> alreadyChecked = newHashMap();
+ hasNonProto2Imports(root, currentlyChecking, alreadyChecked);
+ }
+
+ private boolean hasNonProto2Imports(Protobuf root, Set<Protobuf> currentlyChecking,
+ Map<Protobuf, IsProto2> alreadyChecked) {
+ IsProto2 isProto2 = alreadyChecked.get(root);
+ if (isProto2 != null) {
+ return isProto2 == IsProto2.NO;
+ }
+ currentlyChecking.add(root);
+ Set<Pair<Import, Protobuf>> importsToCheck = newHashSet();
+ boolean hasNonProto2Imports = false;
for (Import anImport : protobufs.importsIn(root)) {
- Resource imported = importedResource(resourceSet, anImport);
- checked.add(imported.getURI());
- if (!protobufs.isProto2(resources.rootOf(imported))) {
- hasNonProto2 = true;
+ Resource imported = imports.importedResource(anImport);
+ if (imported == null) {
+ continue;
+ }
+ Protobuf importedRoot = resources.rootOf(imported);
+ isProto2 = alreadyChecked.get(importedRoot);
+ if (isProto2 != null) {
+ // resource was already checked.
+ if (isProto2 == IsProto2.NO) {
+ hasNonProto2Imports = true;
+ warnNonProto2ImportFoundIn(anImport);
+ }
+ continue;
+ }
+ if (!protobufs.isProto2(importedRoot)) {
+ alreadyChecked.put(importedRoot, IsProto2.NO);
+ hasNonProto2Imports = true;
warnNonProto2ImportFoundIn(anImport);
continue;
}
- resourcesToCheck.add(pair(anImport, imported));
- }
- if (hasNonProto2) {
- return;
- }
- for (Pair<Import, Resource> p : resourcesToCheck) {
- if (hasNonProto2(p, checked, resourceSet)) {
- warnNonProto2ImportFoundIn(p.getFirst());
- break;
- }
- }
- }
-
- private boolean hasNonProto2(Pair<Import, Resource> toCheck, Set<URI> alreadyChecked, ResourceSet resourceSet) {
- Protobuf root = resources.rootOf(toCheck.getSecond());
- if (!protobufs.isProto2(root)) {
- return false;
- }
- List<Pair<Import, Resource>> resourcesToCheck = newArrayList();
- for (Import anImport : protobufs.importsIn(root)) {
- Resource imported = importedResource(resourceSet, anImport);
- if (alreadyChecked.contains(imported.getURI())) {
+ // we have a circular dependency
+ if (currentlyChecking.contains(importedRoot)) {
continue;
}
- if (!protobufs.isProto2(resources.rootOf(imported))) {
- return true;
- }
- resourcesToCheck.add(pair(toCheck.getFirst(), imported));
+ // this is a proto2 file. Need to check its imports.
+ importsToCheck.add(pair(anImport, importedRoot));
}
- for (Pair<Import, Resource> p : resourcesToCheck) {
- if (hasNonProto2(p, alreadyChecked, resourceSet)) {
- return true;
+ for (Pair<Import, Protobuf> importToCheck : importsToCheck) {
+ if (hasNonProto2Imports(importToCheck.getSecond(), currentlyChecking, alreadyChecked)) {
+ hasNonProto2Imports = true;
+ warnNonProto2ImportFoundIn(importToCheck.getFirst());
}
}
- return false;
- }
-
- private Resource importedResource(ResourceSet resourceSet, Import anImport) {
- URI resolvedUri = imports.resolvedUriOf(anImport);
- if (resolvedUri != null) {
- return resourceSets.findResource(resourceSet, resolvedUri);
- }
- return null;
+ isProto2 = hasNonProto2Imports ? IsProto2.NO : IsProto2.YES;
+ alreadyChecked.put(root, isProto2);
+ currentlyChecking.remove(root);
+ return hasNonProto2Imports;
}
private void warnNonProto2ImportFoundIn(Import anImport) {
- acceptWarning(importingNonProto2, anImport, IMPORT__IMPORT_URI, INSIGNIFICANT_INDEX, null);
+ warning(importingNonProto2, anImport, IMPORT__IMPORT_URI, INSIGNIFICANT_INDEX);
}
/**
@@ -132,4 +120,8 @@
error(format(importNotFound, anImport.getImportURI()), IMPORT__IMPORT_URI);
}
}
+
+ private static enum IsProto2 {
+ YES, NO;
+ }
}
