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.

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

Merged
merged 1 commit into from
Jan 14, 2021

Conversation

angelozerr
Copy link
Contributor

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

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),
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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).

Copy link
Contributor

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?

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 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?

Copy link
Contributor

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

Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

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

Copy link
Contributor Author

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.

@angelozerr angelozerr force-pushed the invalidSchemaLocation branch 5 times, most recently from 9028503 to d3dca18 Compare January 11, 2021 15:03
@angelozerr
Copy link
Contributor Author

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.

@angelozerr angelozerr force-pushed the invalidSchemaLocation branch 3 times, most recently from f30f7f8 to 5d80049 Compare January 13, 2021 10:14
@datho7561 datho7561 self-requested a review January 13, 2021 14:48
@angelozerr angelozerr force-pushed the invalidSchemaLocation branch 2 times, most recently from 3d6c5c0 to 79e4800 Compare January 13, 2021 16:22
namespace for the document element root.

See eclipse-lemminx#951

Signed-off-by: azerr <azerr@redhat.com>
@datho7561
Copy link
Contributor

Native image still isn't working; I'll need to take a closer look to figure out why

@datho7561
Copy link
Contributor

native image is not working in HEAD of master, so its likely not caused by this PR.

@datho7561
Copy link
Contributor

I messed up, the native-image version is working, both on master and this PR.

@datho7561 datho7561 merged commit 6e5a906 into eclipse-lemminx:master Jan 14, 2021
@angelozerr angelozerr self-assigned this Jan 15, 2021
@angelozerr angelozerr added this to the 0.15.0 milestone Jan 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants