Fixed issue where real unresolved references were shown as warnings when
having unresolved imports only.
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
index 5b9a99b..f60840b 100644
--- 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
@@ -12,13 +12,13 @@
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;
+import org.eclipse.xtext.validation.ValidationMessageAcceptor;
+import org.junit.*;
+
/**
* Tests for <code>{@link ImportValidator#checkNonProto2Imports(Protobuf)}</code>
*
@@ -74,4 +74,12 @@
validator.checkNonProto2Imports(xtext.root());
verifyNoMoreInteractions(messageAcceptor);
}
+
+ // syntax = "proto2";
+ //
+ // import "093651b0-5676-11e1-b86c-0800200c9a66.proto";
+ @Test public void should_not_add_warnings_if_imported_file_does_not_exist() {
+ validator.checkNonProto2Imports(xtext.root());
+ verifyNoMoreInteractions(messageAcceptor);
+ }
}
diff --git a/com.google.eclipse.protobuf.test/src/com/google/eclipse/protobuf/junit/core/IntegrationTestModule.java b/com.google.eclipse.protobuf.test/src/com/google/eclipse/protobuf/junit/core/IntegrationTestModule.java
index beafd75..c714463 100644
--- a/com.google.eclipse.protobuf.test/src/com/google/eclipse/protobuf/junit/core/IntegrationTestModule.java
+++ b/com.google.eclipse.protobuf.test/src/com/google/eclipse/protobuf/junit/core/IntegrationTestModule.java
@@ -46,7 +46,7 @@
}
File file = protoFile(importUri);
if (!file.exists()) {
- throw new IllegalArgumentException("File: " + importUri + " does not exist.");
+ return; // file does not exist.
}
String resolvedUri = file.toURI().toString();
anImport.setImportURI(resolvedUri);
diff --git a/com.google.eclipse.protobuf.test/src/com/google/eclipse/protobuf/linking/ProtobufDiagnostic_constructor_Test.java b/com.google.eclipse.protobuf.test/src/com/google/eclipse/protobuf/linking/ProtobufDiagnostic_constructor_Test.java
index dbcb6cb..3e03766 100644
--- a/com.google.eclipse.protobuf.test/src/com/google/eclipse/protobuf/linking/ProtobufDiagnostic_constructor_Test.java
+++ b/com.google.eclipse.protobuf.test/src/com/google/eclipse/protobuf/linking/ProtobufDiagnostic_constructor_Test.java
@@ -35,7 +35,7 @@
new ProtobufDiagnostic("1000", null, "message", node);
}
- @Test(expected = NullPointerException.class)
+ @Test(expected = IllegalArgumentException.class)
public void should_throw_exception_if_data_contains_nulls() {
new ProtobufDiagnostic("1000", new String[] { null }, "message", node);
}
diff --git a/com.google.eclipse.protobuf.test/src/com/google/eclipse/protobuf/model/util/Imports_resolvedUriOf_Test.java b/com.google.eclipse.protobuf.test/src/com/google/eclipse/protobuf/model/util/Imports_resolvedUriOf_Test.java
new file mode 100644
index 0000000..f94fb74
--- /dev/null
+++ b/com.google.eclipse.protobuf.test/src/com/google/eclipse/protobuf/model/util/Imports_resolvedUriOf_Test.java
@@ -0,0 +1,59 @@
+/*
+ * 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.model.util;
+
+import static com.google.eclipse.protobuf.junit.core.UnitTestModule.unitTestModule;
+import static com.google.eclipse.protobuf.junit.core.XtextRule.overrideRuntimeModuleWith;
+import static org.hamcrest.core.IsEqual.equalTo;
+import static org.junit.Assert.*;
+import static org.mockito.Mockito.*;
+
+import com.google.eclipse.protobuf.junit.core.*;
+import com.google.eclipse.protobuf.protobuf.Import;
+import com.google.inject.Inject;
+
+import org.eclipse.emf.common.util.URI;
+import org.eclipse.xtext.scoping.impl.ImportUriResolver;
+import org.junit.*;
+
+/**
+ * Tests for <code>{@link Imports#resolvedUriOf(Import)}</code>
+ *
+ * @author alruiz@google.com (Alex Ruiz)
+ */
+public class Imports_resolvedUriOf_Test {
+ @Rule public XtextRule xtext = overrideRuntimeModuleWith(unitTestModule(), new TestModule());
+
+ @Inject private ImportUriResolver uriResolver;
+ @Inject private Imports imports;
+
+ private Import anImport;
+
+ @Before public void setUp() {
+ anImport = mock(Import.class);
+ }
+
+ @Test public void should_return_resolved_URI() {
+ when(uriResolver.apply(anImport)).thenReturn("file:/protos/test.proto");
+ URI uri = imports.resolvedUriOf(anImport);
+ assertThat(uri.path(), equalTo("/protos/test.proto"));
+ }
+
+ @Test public void should_return_null_if_URI_cannot_be_resolved() {
+ when(uriResolver.apply(anImport)).thenReturn(null);
+ URI uri = imports.resolvedUriOf(anImport);
+ assertNull(uri);
+ }
+
+ private static class TestModule extends AbstractTestModule {
+ @Override protected void configure() {
+ mockAndBind(ImportUriResolver.class);
+ }
+ }
+}
diff --git a/com.google.eclipse.protobuf/src/com/google/eclipse/protobuf/linking/ProtobufDiagnostic.java b/com.google.eclipse.protobuf/src/com/google/eclipse/protobuf/linking/ProtobufDiagnostic.java
index 80a5387..d6591a5 100644
--- a/com.google.eclipse.protobuf/src/com/google/eclipse/protobuf/linking/ProtobufDiagnostic.java
+++ b/com.google.eclipse.protobuf/src/com/google/eclipse/protobuf/linking/ProtobufDiagnostic.java
@@ -12,11 +12,11 @@
import static java.util.Arrays.copyOf;
import static org.eclipse.xtext.util.Arrays.contains;
+import com.google.common.base.Objects;
+
import org.eclipse.xtext.diagnostics.*;
import org.eclipse.xtext.nodemodel.INode;
-import com.google.common.base.Objects;
-
/**
* <code>{@link Diagnostic}</code> that supports appending text to its message.
*
@@ -29,9 +29,14 @@
private final INode node;
public ProtobufDiagnostic(String code, String[] data, String message, INode node) {
- validate(data);
+ if (data == null) {
+ throw new NullPointerException("The given array should not be null");
+ }
+ if (contains(data, null)) {
+ throw new IllegalArgumentException("The given array should not contain null elements");
+ }
if (node == null) {
- throw new NullPointerException("node should not be null");
+ throw new NullPointerException("The given node should not be null");
}
this.code = code;
this.data = copyOf(data, data.length);
@@ -40,15 +45,6 @@
this.node = node;
}
- private static void validate(String[] data) {
- if (data == null) {
- throw new NullPointerException("data should not be a null array");
- }
- if (contains(data, null)) {
- throw new NullPointerException("data should not contain null");
- }
- }
-
@Override public String getCode() {
return code;
}
@@ -66,7 +62,7 @@
}
/**
- * Appends the given text to the message of this diagnostic.
+ * Appends the given text to this diagnostic's message.
* @param s the text to append.
*/
public void appendToMessage(String s) {
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 408118c..d308b3f 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
@@ -12,16 +12,16 @@
import static com.google.eclipse.protobuf.util.Strings.*;
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;
+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;
+
/**
* Utility methods related to imports.
*
@@ -90,10 +90,10 @@
* @return {@code true} if the URI of the given {@code Import} has been resolved, {@code false} otherwise.
*/
public boolean isResolved(Import anImport) {
- String importUri = anImport.getImportURI();
- if (!isEmpty(importUri)) {
- URI uri = URI.createURI(importUri);
- if (!isEmpty(uri.scheme())) {
+ String uriAsText = anImport.getImportURI();
+ if (!isEmpty(uriAsText)) {
+ URI uri = URI.createURI(uriAsText);
+ if (isResolved(uri)) {
return true;
}
}
@@ -122,6 +122,14 @@
*/
public URI resolvedUriOf(Import anImport) {
String resolvedUri = uriResolver.apply(anImport);
- return (isEmpty(resolvedUri)) ? null : URI.createURI(resolvedUri);
+ if (isEmpty(resolvedUri)) {
+ return null;
+ }
+ URI uri = URI.createURI(resolvedUri);
+ return (isResolved(uri)) ? uri : null;
+ }
+
+ private boolean isResolved(URI uri) {
+ return !isEmpty(uri.scheme());
}
}
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 d852659..878ae52 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
@@ -15,16 +15,16 @@
import static java.lang.String.format;
import static org.eclipse.xtext.util.Tuples.pair;
-import java.util.*;
+import com.google.eclipse.protobuf.model.util.*;
+import com.google.eclipse.protobuf.protobuf.*;
+import com.google.inject.Inject;
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.inject.Inject;
+import java.util.*;
/**
* Verifies that "imports" contain correct values.
diff --git a/com.google.eclipse.protobuf/src/com/google/eclipse/protobuf/validation/ProtobufResourceValidator.java b/com.google.eclipse.protobuf/src/com/google/eclipse/protobuf/validation/ProtobufResourceValidator.java
index f161e5a..648bc90 100644
--- a/com.google.eclipse.protobuf/src/com/google/eclipse/protobuf/validation/ProtobufResourceValidator.java
+++ b/com.google.eclipse.protobuf/src/com/google/eclipse/protobuf/validation/ProtobufResourceValidator.java
@@ -18,7 +18,7 @@
import static org.eclipse.xtext.validation.CheckType.FAST;
import static org.eclipse.xtext.validation.impl.ConcreteSyntaxEValidator.DISABLE_CONCRETE_SYNTAX_EVALIDATOR;
-import java.util.*;
+import com.google.eclipse.protobuf.linking.ProtobufDiagnostic;
import org.apache.log4j.Logger;
import org.eclipse.emf.common.util.Diagnostic;
@@ -29,7 +29,7 @@
import org.eclipse.xtext.util.*;
import org.eclipse.xtext.validation.*;
-import com.google.eclipse.protobuf.linking.ProtobufDiagnostic;
+import java.util.*;
/**
* Adds support for converting scoping errors into warnings if non-proto2 files are imported.
@@ -48,51 +48,18 @@
List<Issue> result = newArrayListWithExpectedSize(resource.getErrors().size() + resource.getWarnings().size());
try {
IAcceptor<Issue> acceptor = createAcceptor(result);
- boolean hasNonProto2Import = false;
- for (EObject element : resource.getContents()) {
- try {
- if (monitor.isCanceled()) {
- return null;
- }
- Diagnostic diagnostic = getDiagnostician().validate(element, validationOptions(resource, mode, monitor));
- if (!diagnostic.getChildren().isEmpty()) {
- for (Diagnostic child : diagnostic.getChildren()) {
- if (importingNonProto2.equals(child.getMessage())) {
- hasNonProto2Import = true;
- }
- issueFromEValidatorDiagnostic(child, acceptor);
- }
- } else {
- issueFromEValidatorDiagnostic(diagnostic, acceptor);
- }
- } catch (RuntimeException e) {
- log.error(e.getMessage(), e);
- }
+ Status status = delegateValidationToDiagnostician(resource, mode, monitor, acceptor);
+ if (status.isCanceled()) {
+ return null;
}
if (mode.shouldCheck(FAST)) {
- for (Resource.Diagnostic error : resource.getErrors()) {
- if (monitor.isCanceled()) {
- return null;
- }
- Severity severity = ERROR;
- if (hasNonProto2Import && isUnresolveReferenceError(error)) {
- severity = WARNING;
- ProtobufDiagnostic d = (ProtobufDiagnostic) error;
- if (!d.getMessage().endsWith(scopingError)) {
- if (!d.getMessage().endsWith(".")) {
- d.appendToMessage(".");
- }
- d.appendToMessage(" ");
- d.appendToMessage(scopingError);
- }
- }
- issueFromXtextResourceDiagnostic(error, severity, acceptor);
+ status = createErrors(resource, status.hasProto1Imports(), acceptor, monitor);
+ if (status.isCanceled()) {
+ return null;
}
- for (Resource.Diagnostic warning : resource.getWarnings()) {
- if (monitor.isCanceled()) {
- return null;
- }
- issueFromXtextResourceDiagnostic(warning, WARNING, acceptor);
+ status = createWarnings(resource, acceptor, monitor);
+ if (status.isCanceled()) {
+ return null;
}
}
} catch (RuntimeException e) {
@@ -101,6 +68,21 @@
return result;
}
+ private Status delegateValidationToDiagnostician(Resource resource, CheckMode mode,
+ CancelIndicator monitor, IAcceptor<Issue> acceptor) {
+ Status hasNonProto2Import = Status.OK;
+ for (EObject element : resource.getContents()) {
+ if (monitor.isCanceled()) {
+ return Status.CANCELED;
+ }
+ Diagnostic diagnostic = getDiagnostician().validate(element, validationOptions(resource, mode, monitor));
+ if (convertIssuesToMarkers(acceptor, diagnostic) == Status.PROTO1_IMPORTS_FOUND) {
+ hasNonProto2Import = Status.PROTO1_IMPORTS_FOUND;
+ }
+ }
+ return hasNonProto2Import;
+ }
+
private Map<Object, Object> validationOptions(Resource resource, CheckMode mode, CancelIndicator monitor) {
Map<Object, Object> options = newHashMap();
options.put(KEY, mode);
@@ -113,14 +95,76 @@
return options;
}
- private boolean isUnresolveReferenceError(Resource.Diagnostic error) {
+ private Status convertIssuesToMarkers(IAcceptor<Issue> acceptor, Diagnostic diagnostic) {
+ Status hasNonProto2Import = Status.OK;
+ if (diagnostic.getChildren().isEmpty()) {
+ issueFromEValidatorDiagnostic(diagnostic, acceptor);
+ return hasNonProto2Import;
+ }
+ for (Diagnostic child : diagnostic.getChildren()) {
+ if (importingNonProto2.equals(child.getMessage())) {
+ hasNonProto2Import = Status.PROTO1_IMPORTS_FOUND;
+ }
+ issueFromEValidatorDiagnostic(child, acceptor);
+ }
+ return hasNonProto2Import;
+ }
+
+ private Status createErrors(Resource resource, boolean proto1ImportsFound, IAcceptor<Issue> acceptor,
+ CancelIndicator monitor) {
+ for (Resource.Diagnostic error : resource.getErrors()) {
+ if (monitor.isCanceled()) {
+ return Status.CANCELED;
+ }
+ Severity severity = ERROR;
+ if (proto1ImportsFound && isUnresolvedReferenceError(error)) {
+ severity = WARNING;
+ ProtobufDiagnostic d = (ProtobufDiagnostic) error;
+ String message = d.getMessage();
+ if (message.endsWith(scopingError)) {
+ continue;
+ }
+ if (!message.endsWith(".")) {
+ d.appendToMessage(".");
+ }
+ d.appendToMessage(" ");
+ d.appendToMessage(scopingError);
+ }
+ issueFromXtextResourceDiagnostic(error, severity, acceptor);
+ }
+ return Status.OK;
+ }
+
+ private boolean isUnresolvedReferenceError(Resource.Diagnostic error) {
if (!(error instanceof ProtobufDiagnostic)) {
return false;
}
ProtobufDiagnostic d = (ProtobufDiagnostic) error;
- if (!"org.eclipse.xtext.diagnostics.Diagnostic.Linking".equals(d.getCode())) {
- return false;
+ if ("org.eclipse.xtext.diagnostics.Diagnostic.Linking".equals(d.getCode())) {
+ return error.getMessage().startsWith("Couldn't resolve");
}
- return error.getMessage().startsWith("Couldn't resolve");
+ return false;
+ }
+
+ private Status createWarnings(Resource resource, IAcceptor<Issue> acceptor, CancelIndicator monitor) {
+ for (Resource.Diagnostic warning : resource.getWarnings()) {
+ if (monitor.isCanceled()) {
+ return Status.CANCELED;
+ }
+ issueFromXtextResourceDiagnostic(warning, WARNING, acceptor);
+ }
+ return Status.OK;
+ }
+
+ private static enum Status {
+ OK, CANCELED, PROTO1_IMPORTS_FOUND;
+
+ boolean hasProto1Imports() {
+ return (this == PROTO1_IMPORTS_FOUND);
+ }
+
+ boolean isCanceled() {
+ return (this == CANCELED);
+ }
}
}
\ No newline at end of file
