Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TargetNamespace.1/2 error range fix #761

Merged
merged 1 commit into from
Jun 9, 2020
Merged

TargetNamespace.1/2 error range fix #761

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion ...ain/java/org/eclipse/lemminx/extensions/contentmodel/participants/XMLSchemaErrorCode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,14 @@

import org.apache.xerces.xni.XMLLocator;
import org.eclipse.lemminx.commons.BadLocationException;
import org.eclipse.lemminx.dom.DOMAttr;
import org.eclipse.lemminx.dom.DOMDocument;
import org.eclipse.lemminx.dom.DOMElement;
import org.eclipse.lemminx.dom.DOMNode;
import org.eclipse.lemminx.dom.NoNamespaceSchemaLocation;
import org.eclipse.lemminx.dom.SchemaLocation;
import org.eclipse.lemminx.extensions.contentmodel.participants.codeactions.TargetNamespace_1CodeAction;
import org.eclipse.lemminx.extensions.contentmodel.participants.codeactions.TargetNamespace_2CodeAction;
import org.eclipse.lemminx.extensions.contentmodel.participants.codeactions.cvc_attribute_3CodeAction;
import org.eclipse.lemminx.extensions.contentmodel.participants.codeactions.cvc_complex_type_2_1CodeAction;
import org.eclipse.lemminx.extensions.contentmodel.participants.codeactions.cvc_complex_type_2_3CodeAction;
Expand Down Expand Up @@ -74,6 +77,7 @@ public enum XMLSchemaErrorCode implements IXMLErrorCode {
cvc_maxInclusive_valid("cvc-maxInclusive-valid"), // https://wiki.xmldation.com/Support/validator/cvc-maxinclusive-valid
cvc_minExclusive_valid("cvc-minExclusive-valid"), // https://wiki.xmldation.com/Support/validator/cvc-minexclusive-valid
cvc_minInclusive_valid("cvc-minInclusive-valid"), // https://wiki.xmldation.com/Support/validator/cvc-mininclusive-valid
TargetNamespace_1("TargetNamespace.1"), //
TargetNamespace_2("TargetNamespace.2"), SchemaLocation("SchemaLocation"), schema_reference_4("schema_reference.4"), //
src_element_3("src-element.3");

Expand Down Expand Up @@ -138,7 +142,6 @@ public static Range toLSPRange(XMLLocator location, XMLSchemaErrorCode code, Obj
case cvc_elt_1_a:
case cvc_complex_type_4:
case src_element_3:
case TargetNamespace_2:
return XMLPositionUtility.selectStartTagName(offset, document);
case cvc_complex_type_3_2_2: {
String attrName = getString(arguments[1]);
Expand Down Expand Up @@ -245,6 +248,10 @@ public static Range toLSPRange(XMLLocator location, XMLSchemaErrorCode code, Obj
}
case cvc_type_3_1_2:
return XMLPositionUtility.selectStartTagName(offset, document);
case TargetNamespace_1:
return XMLPositionUtility.selectRootAttributeValue(DOMAttr.XMLNS_ATTR, document);
case TargetNamespace_2:
return XMLPositionUtility.selectRootStartTag(document);
default:
}
return null;
Expand All @@ -260,5 +267,7 @@ public static void registerCodeActionParticipants(Map<String, ICodeActionPartici
codeActions.put(cvc_complex_type_3_2_2.getCode(), new cvc_complex_type_3_2_2CodeAction());
codeActions.put(cvc_enumeration_valid.getCode(), new cvc_enumeration_validCodeAction());
codeActions.put(cvc_complex_type_2_1.getCode(), new cvc_complex_type_2_1CodeAction());
codeActions.put(TargetNamespace_1.getCode(), new TargetNamespace_1CodeAction());
codeActions.put(TargetNamespace_2.getCode(), new TargetNamespace_2CodeAction());
}
}
70 changes: 70 additions & 0 deletions ...lemminx/extensions/contentmodel/participants/codeactions/TargetNamespace_1CodeAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
/**
* Copyright (c) 2020 Red Hat Inc. and others.
* All rights reserved. This program and the accompanying materials
* are made available under the terms of the Eclipse Public License v2.0
* which accompanies this distribution, and is available at
* http://www.eclipse.org/legal/epl-v20.html
*
* SPDX-License-Identifier: EPL-2.0
*
* Contributors:
* Red Hat Inc. - initial API and implementation
*/
package org.eclipse.lemminx.extensions.contentmodel.participants.codeactions;

import java.util.List;
import java.util.regex.Pattern;
import java.util.regex.Matcher;

import org.eclipse.lemminx.commons.CodeActionFactory;
import org.eclipse.lemminx.dom.DOMDocument;
import org.eclipse.lemminx.services.extensions.ICodeActionParticipant;
import org.eclipse.lemminx.services.extensions.IComponentProvider;
import org.eclipse.lemminx.settings.SharedSettings;
import org.eclipse.lemminx.utils.StringUtils;
import org.eclipse.lsp4j.CodeAction;
import org.eclipse.lsp4j.Diagnostic;
import org.eclipse.lsp4j.Range;

/**
* CodeAction to replace an incorrect namespace in an .xml document.
*
* Changes the value of the xmlns attribute of the root element of the .xml
* document to the declared namespace of the referenced .xsd document.
*/
public class TargetNamespace_1CodeAction implements ICodeActionParticipant {
datho7561 marked this conversation as resolved.
Show resolved Hide resolved

private static final Pattern NAMESPACE_EXTRACTOR = Pattern.compile("'([^']+)'\\.");

@Override
public void doCodeAction(Diagnostic diagnostic, Range range, DOMDocument document, List<CodeAction> codeActions,
SharedSettings sharedSettings, IComponentProvider componentProvider) {

String namespace = extractNamespace(diagnostic.getMessage());
datho7561 marked this conversation as resolved.
Show resolved Hide resolved
if (StringUtils.isEmpty(namespace)) {
return;
}
String quote = sharedSettings.getPreferences().getQuotationAsString();
// @formatter:off
CodeAction replaceNamespace = CodeActionFactory.replace(
"Replace with '" + namespace + "'",
diagnostic.getRange(),
quote + namespace + quote,
document.getTextDocument(),
diagnostic);
// @formatter:on
codeActions.add(replaceNamespace);
}

private static String extractNamespace(String diagnosticMessage) {
// The error message has this form:
// TargetNamespace.1: Expecting namespace 'NaN', but the target namespace of the
// schema document is 'http://two-letter-name'.
Matcher nsMatcher = NAMESPACE_EXTRACTOR.matcher(diagnosticMessage);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My worry is what if the namespace contains a ' (not url encoded like %XX) character?

@angelozerr do we need to worry about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I looked at the specification to see if ' is allowed in a namespace: https://www.w3.org/TR/xml/#NT-Name
My understanding from this document is that it isn't.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Xerces doesn't complain when you write targetNamespace="'", so I think we should take care of even if it's not a common usecases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working on it and unit tests for it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I implemented it so that it will work with ' and escapes it when it occurs, but it doesn't recognize the escaped version of ', so it still raises TargetNamespace.1:

DoesntFixErrorQuotes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed the code that implements the above (with tests) into this PR. Right now, it always tries to escape single quotes. I left it as a separate commit in case it we want to revert it. Other options could be:

  • Always use double quotes when declaring xmlns if there is a single quote embedded in the targetNamespace
  • Don't give a code action if single quotes are prefered and there are single quotes in the targetNamespace

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^This has been reverted back to the code that doesn't deal with single quotes in the namespace

if (nsMatcher.find()) {
return nsMatcher.group(1);
}
return null;
}

}
79 changes: 79 additions & 0 deletions ...lemminx/extensions/contentmodel/participants/codeactions/TargetNamespace_2CodeAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
/**
* Copyright (c) 2020 Red Hat Inc. and others.
* All rights reserved. This program and the accompanying materials
* are made available under the terms of the Eclipse Public License v2.0
* which accompanies this distribution, and is available at
* http://www.eclipse.org/legal/epl-v20.html
*
* SPDX-License-Identifier: EPL-2.0
*
* Contributors:
* Red Hat Inc. - initial API and implementation
*/
package org.eclipse.lemminx.extensions.contentmodel.participants.codeactions;

import java.util.List;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

import org.eclipse.lemminx.commons.CodeActionFactory;
import org.eclipse.lemminx.dom.DOMDocument;
import org.eclipse.lemminx.dom.DOMNode;
import org.eclipse.lemminx.services.extensions.ICodeActionParticipant;
import org.eclipse.lemminx.services.extensions.IComponentProvider;
import org.eclipse.lemminx.settings.SharedSettings;
import org.eclipse.lemminx.utils.StringUtils;
import org.eclipse.lemminx.utils.XMLPositionUtility;
import org.eclipse.lsp4j.CodeAction;
import org.eclipse.lsp4j.Diagnostic;
import org.eclipse.lsp4j.Position;
import org.eclipse.lsp4j.Range;

/**
* Code action to add the missing xmlns declaration to the root element in an
* .xml document.
*
* Finds the namespace of the referenced .xsd. Adds the xmlns attribute to the
* root of .xml, and sets its value to the .xsd namespace.
*/
public class TargetNamespace_2CodeAction implements ICodeActionParticipant {
datho7561 marked this conversation as resolved.
Show resolved Hide resolved

private static final Pattern NAMESPACE_EXTRACTOR = Pattern.compile("'([^']+)'\\.");

@Override
public void doCodeAction(Diagnostic diagnostic, Range range, DOMDocument document, List<CodeAction> codeActions,
SharedSettings sharedSettings, IComponentProvider componentProvider) {

String namespace = extractNamespace(diagnostic.getMessage());
if (StringUtils.isEmpty(namespace)) {
return;
}
DOMNode root = document.getDocumentElement();
if (root == null) {
return;
}
Position tagEnd = XMLPositionUtility.selectStartTagName(root).getEnd();
String quote = sharedSettings.getPreferences().getQuotationAsString();
// @formatter:off
CodeAction addNamespaceDecl = CodeActionFactory.insert(
"Declare '" + namespace + "' as the namespace",
tagEnd,
" xmlns=" + quote + namespace + quote,
document.getTextDocument(),
diagnostic);
// @formatter:on
codeActions.add(addNamespaceDecl);
}

private static String extractNamespace(String diagnosticMessage) {
// The error message has this form:
// TargetNamespace.2: Expecting no namespace, but the schema document has a
// target namespace of 'http://docbook.org/ns/docbook'.
Matcher nsMatcher = NAMESPACE_EXTRACTOR.matcher(diagnosticMessage);
if (nsMatcher.find()) {
return nsMatcher.group(1);
}
return null;
}

}
28 changes: 28 additions & 0 deletions org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/utils/XMLPositionUtility.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,34 @@ public static Range selectRootStartTag(DOMDocument document) {
return selectStartTagName(root);
}

/**
* Finds the root element of the given document and returns the
* attribute value <code>Range</code> for the attribute
* <code>attrName</code>.
*
* If <code>attrName</code> is not declared then null is returned.
*
* @param attrName The name of the attribute to find the range of the value for
* @param document The document to use the root element of
* @return The range in <code>document</code> where the declared value of
* attribute <code>attrName</code> resides (including quotations),
* or null if the attriubte is not declared.
*/
public static Range selectRootAttributeValue(String attrName, DOMDocument document) {
datho7561 marked this conversation as resolved.
Show resolved Hide resolved
DOMNode root = document.getDocumentElement();
if (root == null) {
root = document.getChild(0);
}
if (root == null) {
return null;
}
DOMAttr attr = root.getAttributeNode(attrName);
if (attr == null) {
return null;
}
return selectAttributeValue(attr);
}

public static Range selectStartTagName(int offset, DOMDocument document) {
DOMNode element = document.findNodeAt(offset);
if (element != null) {
Expand Down
66 changes: 66 additions & 0 deletions ...x/src/test/java/org/eclipse/lemminx/extensions/contentmodel/XMLSchemaDiagnosticsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@
import org.eclipse.lemminx.XMLAssert;
import org.eclipse.lemminx.extensions.contentmodel.participants.XMLSchemaErrorCode;
import org.eclipse.lemminx.extensions.contentmodel.settings.ContentModelSettings;
import org.eclipse.lemminx.settings.EnforceQuoteStyle;
import org.eclipse.lemminx.settings.QuoteStyle;
import org.eclipse.lemminx.settings.SharedSettings;
import org.eclipse.lsp4j.Diagnostic;
import org.junit.jupiter.api.Test;

Expand Down Expand Up @@ -577,6 +580,69 @@ public void cvc_complex_type_2_2_withText() throws Exception {
testDiagnosticsFor(xml, diagnosticBob, diagnostic_cvc_2_2);
}

@Test
public void testTargetNamespace_1Normal() throws Exception {
String xml = "<?xml version=\"1.0\" encoding=\"UTF-8\" ?>\n" + //
"<?xml-model href=\"src/test/resources/xsd/two-letter-name.xsd\"?>\n" + //
"<two-letter-name xmlns=\"BAD_NS\">Io</two-letter-name>";
Diagnostic targetNamespace = d(2, 23, 2, 31, XMLSchemaErrorCode.TargetNamespace_1, "TargetNamespace.1: Expecting namespace 'BAD_NS', but the target namespace of the schema document is 'http://two-letter-name'.");
testDiagnosticsFor(xml,
targetNamespace,
d(2, 1, 2, 16, XMLSchemaErrorCode.cvc_elt_1_a, "cvc-elt.1.a: Cannot find the declaration of element 'two-letter-name'.")
);
testCodeActionsFor(xml, targetNamespace, ca(targetNamespace, te(2, 23, 2, 31, "\"http://two-letter-name\"")));
}

@Test
public void testTargetNamespace_1ShortNS() throws Exception {
String xml = "<?xml version=\"1.0\" encoding=\"UTF-8\" ?>\n" + //
"<?xml-model href=\"src/test/resources/xsd/two-letter-name.xsd\"?>\n" + //
"<two-letter-name xmlns=\"_\">Io</two-letter-name>";
Diagnostic targetNamespace = d(2, 23, 2, 26, XMLSchemaErrorCode.TargetNamespace_1, "TargetNamespace.1: Expecting namespace '_', but the target namespace of the schema document is 'http://two-letter-name'.");
testDiagnosticsFor(xml,
targetNamespace,
d(2, 1, 2, 16, XMLSchemaErrorCode.cvc_elt_1_a, "cvc-elt.1.a: Cannot find the declaration of element 'two-letter-name'.")
);
testCodeActionsFor(xml, targetNamespace, ca(targetNamespace, te(2, 23, 2, 26, "\"http://two-letter-name\"")));
}

@Test
public void testTargetNamespace_1SingleQuotes() throws Exception {
String xml = "<?xml version=\"1.0\" encoding=\"UTF-8\" ?>\n" + //
"<?xml-model href=\"src/test/resources/xsd/two-letter-name.xsd\"?>\n" + //
"<two-letter-name xmlns=\"_\">Io</two-letter-name>";
SharedSettings settings = new SharedSettings();
settings.getFormattingSettings().setEnforceQuoteStyle(EnforceQuoteStyle.preferred);
settings.getPreferences().setQuoteStyle(QuoteStyle.singleQuotes);
Diagnostic targetNamespace = d(2, 23, 2, 26, XMLSchemaErrorCode.TargetNamespace_1, "TargetNamespace.1: Expecting namespace '_', but the target namespace of the schema document is 'http://two-letter-name'.");
testCodeActionsFor(xml, targetNamespace, settings, ca(targetNamespace, te(2, 23, 2, 26, "'http://two-letter-name'")));
}

@Test
public void testTargetNamespace_2() throws Exception {
String xml = "<?xml version=\"1.0\" encoding=\"UTF-8\" ?>\n" + //
"<?xml-model href=\"src/test/resources/xsd/two-letter-name.xsd\"?>\n" + //
"<two-letter-name>Io</two-letter-name>";
Diagnostic targetNamespace = d(2, 1, 2, 16, XMLSchemaErrorCode.TargetNamespace_2, "TargetNamespace.2: Expecting no namespace, but the schema document has a target namespace of 'http://two-letter-name'.");
testDiagnosticsFor(xml,
targetNamespace,
d(2, 1, 2, 16, XMLSchemaErrorCode.cvc_elt_1_a, "cvc-elt.1.a: Cannot find the declaration of element 'two-letter-name'.")
);
testCodeActionsFor(xml, targetNamespace, ca(targetNamespace, te(2, 16, 2, 16, " xmlns=\"http://two-letter-name\"")));
}

@Test
public void testTargetNamespace_2SingleQuotes() throws Exception {
String xml = "<?xml version=\"1.0\" encoding=\"UTF-8\" ?>\n" + //
"<?xml-model href=\"src/test/resources/xsd/two-letter-name.xsd\"?>\n" + //
"<two-letter-name>Io</two-letter-name>";
SharedSettings settings = new SharedSettings();
settings.getFormattingSettings().setEnforceQuoteStyle(EnforceQuoteStyle.preferred);
settings.getPreferences().setQuoteStyle(QuoteStyle.singleQuotes);
Diagnostic targetNamespace = d(2, 1, 2, 16, XMLSchemaErrorCode.TargetNamespace_2, "TargetNamespace.2: Expecting no namespace, but the schema document has a target namespace of 'http://two-letter-name'.");
testCodeActionsFor(xml, targetNamespace, settings, ca(targetNamespace, te(2, 16, 2, 16, " xmlns='http://two-letter-name'")));
}

private static void testDiagnosticsFor(String xml, Diagnostic... expected) {
XMLAssert.testDiagnosticsFor(xml, "src/test/resources/catalogs/catalog.xml", expected);
}
Expand Down
10 changes: 10 additions & 0 deletions org.eclipse.lemminx/src/test/resources/xsd/two-letter-name.xsd
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<?xml version="1.0" encoding="UTF-8"?>
<xs:schema xmlns:xs="http://www.w3.org/2001/XMLSchema" targetNamespace="http://two-letter-name">
<xs:element name="two-letter-name">
<xs:simpleType>
<xs:restriction base="xs:token">
<xs:pattern value="[A-Z][a-z]"></xs:pattern>
</xs:restriction>
</xs:simpleType>
</xs:element>
</xs:schema>