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

Download external resources preference #1155

Merged

Conversation

angelozerr
Copy link
Contributor

Download external resources preference

Signed-off-by: azerr azerr@redhat.com

@angelozerr
Copy link
Contributor Author

This PR requires the vscode-xml PR redhat-developer/vscode-xml#640

It starts working with DTD referenced in DOCTYPE. I need to support correctly DTD inside xml-mode, and referenced XSD.

After that I will need to support XSD and DTD validation (I mean NOT XML validation based on DTD, XSD) .

@angelozerr angelozerr force-pushed the downloadExternalRessource branch from 32d6478 to 700704b Compare January 26, 2022 09:57
@angelozerr
Copy link
Contributor Author

angelozerr commented Jan 26, 2022

At first please don't see my ugly code, it's not cleaned and a lot of tests are failing.

I improved dragisticly the cache resource manager. Now the error is (should be) displayed on the referenced grammar and not in the XMLroot element (when there are loading information or error of downloading).

I introduced a new settings "xml.downloadExternalRessources.enabled": true which is set to true by default and false in an untrusted workspace (for vscode-xml).

When "xml.downloadExternalRessources.enabled" is set to false and the referenced grammar (XSD,DTD) is on the cache of lemminx, this settings must NOT disable validation based on this grammar.

Here a demo with XML which references a maven XSD.

My current context is:

  • http://maven.apache.org/POM/4.0.0 is in my lemminx cache. It means that if I enable/disable "xml.downloadExternalRessource" it should continue to validate correctly the XML based on XSD.
  • https://maven.apache.org/POM/4.0.0 is in NOT in my lemminx cache. It means that if I disable "xml.downloadExternalRessource" we have an error which says that download is forbidden.

DownloadExternalResourceDemo

It should work for XML with DOCTYPE, xsi:schemaLocation, and xml-model -for XSD and DTD both. I need to work for XSD, DTD files (when XSD file references another XSD, DTD)

@angelozerr angelozerr force-pushed the downloadExternalRessource branch from 700704b to 74805a5 Compare January 26, 2022 13:06
@rgrunber
Copy link
Contributor

Seems to be working, though as you've mentioned, XSD and catalogs don't yet respect the option.

@angelozerr angelozerr force-pushed the downloadExternalRessource branch 4 times, most recently from b5b2576 to dd76228 Compare January 27, 2022 08:37
@angelozerr
Copy link
Contributor Author

Seems to be working, though as you've mentioned, XSD and catalogs don't yet respect the option.

  • for XSD, it should work(you can check that file is not downloaded), but the error message is not correct, it should report Downloading external resources is disabled. but it report generic message from Xerces:

image

I cannot customize this message because I need the original exception of the error (to know if it's a loading exception, download which is disabled, or a bad url,etc). Xerces doesn't report error with the ex exception that I need https://github.com/apache/xerces2-j/blob/cf0c517a41b31b0242b96ab1af9627a3ab07fcd2/src/org/apache/xerces/impl/xs/traversers/XSDHandler.java#L2063

@angelozerr angelozerr force-pushed the downloadExternalRessource branch from dd76228 to cd11e56 Compare January 27, 2022 10:10
if (contentModelManager.isDownloadExternalResources() != downloadExternalResourcesEnabled) {
contentModelManager.setDownloadExternalResources(downloadExternalResourcesEnabled);
// Validate all opened XML files
validateAllOpenedDocument(context);
Copy link
Contributor

Choose a reason for hiding this comment

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

if download is now disabled, why do we need to revalidate everything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When you have an XML which references a bad DTD, when you refresh the settings, the error message is different. So we need to revalidate.

@angelozerr angelozerr force-pushed the downloadExternalRessource branch from cd11e56 to dd207f4 Compare January 27, 2022 10:39
@fbricon
Copy link
Contributor

fbricon commented Jan 27, 2022

tests fail:


[INFO] Results:

[INFO] 

[ERROR] Failures: 

[ERROR]   XMLSchemaPublishDiagnosticsTest.schemaWithUrlWithCache:147 expected: <2> but was: <1>

[ERROR]   XMLValidationCommandTest.validationAllFilesCommand:254 Unexpected diagnostics:

[Diagnostic [

  range = Range [

    start = Position [

      line = 1

      character = 22

    ]

    end = Position [

      line = 1

      character = 54

    ]

  ]

  severity = Information

  code = Either [

    left = DownloadingResource

    right = null

  ]

  codeDescription = null

  source = "xml"

  message = "The resource 'http://localhost:41853/tag.dtd' is downloading in the cache path '/home/jenkins/.lemminx/cache/http/localhost/41853/tag.dtd'."

  tags = null

  relatedInformation = null

  data = null

], Diagnostic [

  range = Range [

    start = Position [

      line = 2

      character = 1

    ]

    end = Position [

      line = 2

      character = 5

    ]

  ]

  severity = Error

  code = Either [

    left = MSG_ELEMENT_NOT_DECLARED

    right = null

  ]

  codeDescription = null

  source = "xml"

  message = "Element type "root" must be declared."

  tags = null

  relatedInformation = null

  data = null

], Diagnostic [

  range = Range [

    start = Position [

      line = 3

      character = 3

    ]

    end = Position [

      line = 3

      character = 7

    ]

  ]

  severity = Error

  code = Either [

    left = MSG_ELEMENT_NOT_DECLARED

    right = null

  ]

  codeDescription = null

  source = "xml"

  message = "Element type "tags" must be declared."

  tags = null

  relatedInformation = null

  data = null

]] ==> iterable lengths differ, expected: <1> but was: <3>

[ERROR]   CacheResourcesManagerTest.testGetBadResource:124 expected: <The resource 'http://localhost/../../../../../test.txt' cannot be downloaded in the cache path.> but was: <The resource 'http://localhost/../../../../../test.txt' cannot be downloaded in the cache path '/home/jenkins/agent/workspace/lemminx_PR-1155/org.eclipse.lemminx/target/test-cache/cache/http/localhost/../../../../../test.txt'.>

[ERROR] Errors: 

[ERROR]   CacheResourcesManagerTest.testUnavailableCache » CacheResourceDownloaded Error...

[INFO] 

[ERROR] Tests run: 1415, Failures: 3, Errors: 1, Skipped: 3

@angelozerr angelozerr force-pushed the downloadExternalRessource branch 6 times, most recently from 969c420 to 9113c4b Compare January 27, 2022 16:16
@rgrunber
Copy link
Contributor

Once the test failure is addressed I think this is probably ready to merge. Just confirm the settings for external entity resolution and external resource downloading are what you want them to be based on what the client expects.

@angelozerr angelozerr force-pushed the downloadExternalRessource branch from 9113c4b to 7f373b6 Compare January 28, 2022 10:08
Signed-off-by: azerr <azerr@redhat.com>
@angelozerr angelozerr force-pushed the downloadExternalRessource branch from 7f373b6 to 3819f05 Compare January 28, 2022 14:32
@angelozerr angelozerr marked this pull request as ready for review January 28, 2022 16:12
@fbricon fbricon merged commit c6c90b8 into eclipse-lemminx:master Jan 28, 2022
@angelozerr angelozerr added this to the 0.18.3 milestone Jan 31, 2022
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