[bugfix] registered import-uris have precedence #5576

line-o · 2024-12-10T14:00:33Z

When importing stylesheets the URIs are resolved in order

registered import-uris in package repo
XMLDB-locations (relative, absolute and with or without scheme)
any other location is treated as an exteranal source

The test class and its cases were renamed to reflect their purpose. Some inline comments were added to help describing the intent.

follow-up for #5574

adamretter · 2024-12-10T16:01:37Z

I don't think this works... What about the catalog?

line-o · 2024-12-10T20:26:54Z

@adamretter If you have a test case that shows an issue please post it here. Otherwise, I can only judge by the tests that are already present and I can come up with.

line-o · 2024-12-10T20:28:13Z

At least now the registered import uri is really resolved against the repo instead of trying to resolve it via http request.

adamretter · 2024-12-10T23:35:10Z

You're missing the point of the test, please restore the original test

reinhapa · 2024-12-11T06:25:11Z

You're missing the point of the test, please restore the original test

@adamretter could you please explain to us the point that we missing, instead of just telling us to restore a test as it was that we did no understand in the fist place.

As from #5574

@reinhapa Then why not just ask?

I personally would like to learn from that. Just rolling back will not help me (and may others in the future) in getting reasoning behind the current test. as the discussion seems to prove already. I would like see this issue as a chance to improve the test and the understanding for regression it wants to cover.

line-o · 2024-12-11T09:22:15Z

You're missing the point of the test, please restore the original test

The first test case is importing the stylesheet from the repo using the registered import-uri. So "the original test" is reinstated.

     @Test
     public void fromRegisteredImportUri() throws XMLDBException {
         final String xquery = getQuery("http://www.functx.com/functx.xsl");
         final ResourceSet result = existXmldbEmbeddedServer.executeQuery(xquery);
         assertTransformationResult(result);
     }

line-o · 2024-12-11T09:34:58Z

To better understand the current situation and the proposed solution:

The feature, to be able to import stylesheets using an import-uri that is registered in the databases package repository, did not work. The order of URI resolvers prevented that from happening, as the first resolver captured the URI and resolved it as an external resource.

By putting the PkgXsltModuleURIResolver first, all stylesheet import URIs will be matched against the ones registered in the package repo. That way we can be sure that they take precedence.

adamretter · 2024-12-11T12:37:15Z

I would like see this issue as a chance to improve the test

@reinhapa But it doesn't improve the original test. Instead it removes the original test and adds different tests that prove something completely different.

in getting reasoning behind the current test

@reinhapa You could just talk to me. Even after all the nonsense and abuse I have been shovelled lately, I am still here and trying to help. If you want to understand the original purpose of the test, then the obvious thing would be to have a conversation with me (the original author of the test).

did not work

@line-o That is not my impression. It looks to me like this test only started recently failing, before that it was passing just fine.

line-o · 2024-12-11T14:54:36Z

this test only started recently failing, before that it was passing just fine

@adamretter because of curl -i http://www.functx.com/functx.xsl

reinhapa · 2024-12-16T19:29:50Z

exist-core/src/test/java/org/exist/xquery/functions/transform/ImportStylesheetTest.java

-    public void testImportNoScheme() throws XMLDBException {
-        final String xquery = getQuery(moduleLocation);
+    public void fromRegisteredImportUri() throws XMLDBException {
+        final String xquery = getQuery("http://www.functx.com/functx.xsl");


@line-o I found a working document here: https://www.datypic.com/xsl/functx-1.0.1-doc.xsl that we may should use instead...

Absolutely not. The test is about importing a locally available Stylesheet and the fact that it was resolved from an external source was the problem I fixed.

It just coincidentally was also available at that address and not just registered with that namespace URI in exist-db's package repository.
It is non-obvious to use HTTP-addresses to uniquely name packages without also guaranteeing the source is available there but this is the status-quo, not the use-case this test is about and also a different discussion.

see also what @adamretter wrote:

This test was to ensure the loading of XSLT imports from within EXPath packages.

dizzzz · 2024-12-17T07:31:06Z

@line-o by default curl does not follow redirects; I think the -L flag will make tha happen...

curl -Li http://www.functx.com/functx.xsl

(cant check right now)

If so, we could consider if we can enforce the HTTP-resolver in the test to follow redirects too?

line-o · 2024-12-17T07:37:51Z

But the test is clearly about importing the Stylesheet from within the db. The fact that an HTTP connection es even attempted is a failure.
The test would need to test for that as well. Sadly, it's easier said than done. We would need to either mock a lot or spy on the expected resolver to be called.

adamretter · 2024-12-17T08:16:15Z

Absolutely not. The test is about importing a locally available Stylesheet and the fact that it was resolved from an external source was the problem I fixed.

But the test is clearly about importing the Stylesheet from within the db.

@line-o Nope! That is absolutely not what I wrote this test for. This test was to ensure the loading of XSLT imports from within EXPath packages. This PR deletes that work and important use-case and goes in a completely different direction. This PR is a regression.

The correct approach would have been to fix this in the XML Resolver and/or EXPath package resolver.

line-o · 2024-12-17T08:52:23Z

This test was to ensure the loading of XSLT imports from within EXPath packages.

OK, so we are in agreement here, good. The test case to do that is back in now that this feature is fixed and more cases were added.

The correct approach would have been to fix this in the XML Resolver and/or EXPath package resolver.

This is what I did, by changing the order of the resolvers everything is in order.

line-o · 2024-12-17T09:01:22Z

The http-resolver should not be used for any of the test cases within ImportStylesheetTest. Maybe ImportLocalStylesheetTest would communicate that clearer?

When importing stylesheets the URIs are resolved in order - registered import-uris in package repo - XMLDB-locations (relative, absolute and with or without scheme) - any other location is treated as an exteranal source The test class and its cases were renamed to reflect their purpose. Some inline comments were added to help describing the intent.

sonarqubecloud · 2024-12-30T16:56:24Z

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

line-o requested a review from a team as a code owner December 10, 2024 14:00

line-o added bug issue confirmed as bug needs 6.x.x backport labels Dec 10, 2024

line-o mentioned this pull request Dec 10, 2024

[6.x.x] registered import-uris have precedence #5577

Open

line-o force-pushed the hotfix/transform-import branch from f0c6961 to 401ac6e Compare December 10, 2024 15:07

reinhapa requested changes Dec 16, 2024

View reviewed changes

line-o requested review from reinhapa and a team December 17, 2024 07:41

dizzzz approved these changes Dec 20, 2024

View reviewed changes

dizzzz requested a review from a team December 20, 2024 19:54

line-o force-pushed the hotfix/transform-import branch from 401ac6e to adbb94b Compare December 30, 2024 16:33

reinhapa approved these changes Dec 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[bugfix] registered import-uris have precedence #5576

[bugfix] registered import-uris have precedence #5576

line-o commented Dec 10, 2024 •

edited

Loading

adamretter commented Dec 10, 2024

line-o commented Dec 10, 2024

line-o commented Dec 10, 2024

adamretter commented Dec 10, 2024

reinhapa commented Dec 11, 2024

line-o commented Dec 11, 2024 •

edited

Loading

line-o commented Dec 11, 2024

adamretter commented Dec 11, 2024 •

edited

Loading

line-o commented Dec 11, 2024

reinhapa Dec 16, 2024

line-o Dec 17, 2024

line-o Dec 17, 2024 •

edited

Loading

line-o Dec 17, 2024

dizzzz commented Dec 17, 2024

line-o commented Dec 17, 2024

adamretter commented Dec 17, 2024

line-o commented Dec 17, 2024

line-o commented Dec 17, 2024

sonarqubecloud bot commented Dec 30, 2024

[bugfix] registered import-uris have precedence #5576

Are you sure you want to change the base?

[bugfix] registered import-uris have precedence #5576

Conversation

line-o commented Dec 10, 2024 • edited Loading

adamretter commented Dec 10, 2024

line-o commented Dec 10, 2024

line-o commented Dec 10, 2024

adamretter commented Dec 10, 2024

reinhapa commented Dec 11, 2024

line-o commented Dec 11, 2024 • edited Loading

line-o commented Dec 11, 2024

adamretter commented Dec 11, 2024 • edited Loading

line-o commented Dec 11, 2024

reinhapa Dec 16, 2024

Choose a reason for hiding this comment

line-o Dec 17, 2024

Choose a reason for hiding this comment

line-o Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

line-o Dec 17, 2024

Choose a reason for hiding this comment

dizzzz commented Dec 17, 2024

line-o commented Dec 17, 2024

adamretter commented Dec 17, 2024

line-o commented Dec 17, 2024

line-o commented Dec 17, 2024

sonarqubecloud bot commented Dec 30, 2024

Quality Gate passed

line-o commented Dec 10, 2024 •

edited

Loading

line-o commented Dec 11, 2024 •

edited

Loading

adamretter commented Dec 11, 2024 •

edited

Loading

line-o Dec 17, 2024 •

edited

Loading