-
Notifications
You must be signed in to change notification settings - Fork 93
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.
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
Disable XSD validation when xsi:schemaLocation doesn't declare the hint for the document element root. #953
Disable XSD validation when xsi:schemaLocation doesn't declare the hint for the document element root. #953
Conversation
e770ac1
to
da6f4b3
Compare
da6f4b3
to
df229d2
Compare
XMLAssert.testPublishDiagnosticsFor(xml, fileURI, configuration, pd(fileURI, // | ||
new Diagnostic(r(2, 20, 2, 40), | ||
"SchemaLocation: schemaLocation value = 'http://invoice.xsd' must have even number of URI's.", | ||
new Diagnostic(r(2, 20, 2, 79), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did the test fail before this change? If so, why? I'll also take a look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI build is working. That's very strange.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test works, but the old test should also work (i.e. at least the "odd number of URIs" error should still be present).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see why this is implemented this way. A work around might be to move "xsi:schemaLocation even number of URI" validation into syntax validation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This kind of error is managed by Xerces when XSD validation is enabled, so I think it can be hard to do that. It's not a syntax problem but a XSD (syntax) problem.
I noticed that WTP XML validator disable XSD when:
- the root element namespaceURI doesn't find the XSD to use in the xsi:schemaLocation(done in this PR)
- and if the XSD is found, it check that XSD exists, in this case it disables the XSD validation.
I wonder if we should manage those 2 behaviors with a settings (ex : xml.validation.schema.disableWhenInvalid)
@fbricon what do you think about that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for a new setting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the setting be a part of this PR, or in a new PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be part of this PR, but I need to find a good name for this settings. For the moment I though about xml.validation.disableXSDValidationWhenNotFound
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Today we have xml.validation.schema
which is a boolean, I wonder if we could update this setting with 3 values:
- true
- false
- whenXSDUriIsValid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fbricon could you give us your opinion please about this settings.
9028503
to
d3dca18
Compare
This PR requires the PR on vscode-xml redhat-developer/vscode-xml#390 where you can find a documentation about the new settings xml.validation.schema.enabled. |
f30f7f8
to
5d80049
Compare
...emminx/src/main/java/org/eclipse/lemminx/extensions/contentmodel/settings/SchemaEnabled.java
Outdated
Show resolved
Hide resolved
...emminx/src/main/java/org/eclipse/lemminx/extensions/contentmodel/settings/SchemaEnabled.java
Show resolved
Hide resolved
...nx/src/main/java/org/eclipse/lemminx/extensions/contentmodel/settings/XMLSchemaSettings.java
Outdated
Show resolved
Hide resolved
...nx/src/main/java/org/eclipse/lemminx/extensions/contentmodel/settings/XMLSchemaSettings.java
Show resolved
Hide resolved
3d6c5c0
to
79e4800
Compare
namespace for the document element root. See eclipse-lemminx#951 Signed-off-by: azerr <azerr@redhat.com>
Native image still isn't working; I'll need to take a closer look to figure out why |
native image is not working in HEAD of master, so its likely not caused by this PR. |
I messed up, the native-image version is working, both on master and this PR. |
Disable XSD validation when xsi:schemaLocation doesn't declare the hint for the document element root.
See #951
Signed-off-by: azerr azerr@redhat.com