You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2020/02/19 16:09:43 UTC
[GitHub] [maven-doxia-sitetools] elharo opened a new pull request #10:
[DOXIASITETOOLS-109] handle more URI formats
elharo opened a new pull request #10: [DOXIASITETOOLS-109] handle more URI formats
URL: https://github.com/apache/maven-doxia-sitetools/pull/10
@hboutemy Truly this method needs to be burned to the ground but for the moment, this PR recognizes a number of cases we don't correctly handle now and returns the default `to` URL instead of the mangling the current code does with those.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-doxia-sitetools] elharo commented on a change in pull
request #10: [DOXIASITETOOLS-109] handle more URI formats
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #10: [DOXIASITETOOLS-109] handle more URI formats
URL: https://github.com/apache/maven-doxia-sitetools/pull/10#discussion_r386107854
##########
File path: doxia-integration-tools/src/test/java/org/apache/maven/doxia/tools/DefaultSiteToolTest.java
##########
@@ -56,4 +72,45 @@ public void testGetNormalizedPath()
assertEquals( "file:/Documents and Settings/",
DefaultSiteTool.getNormalizedPath( "file://Documents and Settings/" ) );
}
+
+ @SuppressWarnings("deprecation")
+ @Test
+ public void testGetRelativePath()
+ {
+ assertEquals(
+ ".." + File.separator + "bar.html",
+ tool.getRelativePath("http://example.com/foo/bar.html", "http://example.com/foo/baz.html"));
+ }
+
+ @SuppressWarnings("deprecation")
+ @Test
+ public void testGetRelativePath_same()
+ {
+ assertTrue(
+ tool.getRelativePath( "http://example.com/foo/bar.html", "http://example.com/foo/bar.html" ).isEmpty() );
+ }
+
+ @SuppressWarnings("deprecation")
+ @Test
+ public void testGetRelativePath_differentSchemes() {
+ assertEquals(
+ "scp://example.com/foo/bar.html",
+ tool.getRelativePath( "scp://example.com/foo/bar.html", "http://example.com/foo/bar.html" ) );
+ assertEquals(
+ "file:///tmp/bloop",
+ tool.getRelativePath( "file:///tmp/bloop", "scp://localhost:/tmp/blop" ) );
Review comment:
That's a should, not a MUST. This is a case of being liberal in what we accept. I agree that we should not emit such a weird URL, but we should handle it if someone gives one to us.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-doxia-sitetools] michael-o commented on a change in pull
request #10: [DOXIASITETOOLS-109] handle more URI formats
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #10: [DOXIASITETOOLS-109] handle more URI formats
URL: https://github.com/apache/maven-doxia-sitetools/pull/10#discussion_r381549358
##########
File path: doxia-integration-tools/src/test/java/org/apache/maven/doxia/tools/DefaultSiteToolTest.java
##########
@@ -56,4 +72,45 @@ public void testGetNormalizedPath()
assertEquals( "file:/Documents and Settings/",
DefaultSiteTool.getNormalizedPath( "file://Documents and Settings/" ) );
}
+
+ @SuppressWarnings("deprecation")
+ @Test
+ public void testGetRelativePath()
+ {
+ assertEquals(
+ ".." + File.separator + "bar.html",
+ tool.getRelativePath("http://example.com/foo/bar.html", "http://example.com/foo/baz.html"));
+ }
+
+ @SuppressWarnings("deprecation")
+ @Test
+ public void testGetRelativePath_same()
+ {
+ assertTrue(
+ tool.getRelativePath( "http://example.com/foo/bar.html", "http://example.com/foo/bar.html" ).isEmpty() );
+ }
+
+ @SuppressWarnings("deprecation")
+ @Test
+ public void testGetRelativePath_differentSchemes() {
+ assertEquals(
+ "scp://example.com/foo/bar.html",
+ tool.getRelativePath( "scp://example.com/foo/bar.html", "http://example.com/foo/bar.html" ) );
+ assertEquals(
+ "file:///tmp/bloop",
+ tool.getRelativePath( "file:///tmp/bloop", "scp://localhost:/tmp/blop" ) );
Review comment:
Chapter 3.2.3: URI producers and
normalizers should omit the port component and its ":" delimiter if
port is empty or if its value would be the same as that of the
scheme's default.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-doxia-sitetools] elharo commented on a change in pull
request #10: [DOXIASITETOOLS-109] handle more URI formats
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #10: [DOXIASITETOOLS-109] handle more URI formats
URL: https://github.com/apache/maven-doxia-sitetools/pull/10#discussion_r381530085
##########
File path: doxia-integration-tools/src/test/java/org/apache/maven/doxia/tools/DefaultSiteToolTest.java
##########
@@ -56,4 +72,45 @@ public void testGetNormalizedPath()
assertEquals( "file:/Documents and Settings/",
DefaultSiteTool.getNormalizedPath( "file://Documents and Settings/" ) );
}
+
+ @SuppressWarnings("deprecation")
+ @Test
+ public void testGetRelativePath()
+ {
+ assertEquals(
+ ".." + File.separator + "bar.html",
+ tool.getRelativePath("http://example.com/foo/bar.html", "http://example.com/foo/baz.html"));
+ }
+
+ @SuppressWarnings("deprecation")
+ @Test
+ public void testGetRelativePath_same()
+ {
+ assertTrue(
+ tool.getRelativePath( "http://example.com/foo/bar.html", "http://example.com/foo/bar.html" ).isEmpty() );
+ }
+
+ @SuppressWarnings("deprecation")
+ @Test
+ public void testGetRelativePath_differentSchemes() {
+ assertEquals(
+ "scp://example.com/foo/bar.html",
+ tool.getRelativePath( "scp://example.com/foo/bar.html", "http://example.com/foo/bar.html" ) );
+ assertEquals(
+ "file:///tmp/bloop",
+ tool.getRelativePath( "file:///tmp/bloop", "scp://localhost:/tmp/blop" ) );
Review comment:
surprisingly it is per RFC 3986. The port production is
The port subcomponent of authority is designated by an optional port
number in decimal following the host and delimited from it by a
single colon (":") character.
port = *DIGIT
I copied this case from https://issues.apache.org/jira/browse/DOXIASITETOOLS-110
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-doxia-sitetools] elharo commented on a change in pull
request #10: [DOXIASITETOOLS-109] handle more URI formats
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #10: [DOXIASITETOOLS-109] handle more URI formats
URL: https://github.com/apache/maven-doxia-sitetools/pull/10#discussion_r381534326
##########
File path: doxia-integration-tools/src/main/java/org/apache/maven/doxia/tools/DefaultSiteTool.java
##########
@@ -174,20 +173,34 @@ public Artifact getSkinArtifactFromRepository( ArtifactRepository localRepositor
return artifact;
}
- /** {@inheritDoc} */
public Artifact getDefaultSkinArtifact( ArtifactRepository localRepository,
List<ArtifactRepository> remoteArtifactRepositories )
throws SiteToolException
{
return getSkinArtifactFromRepository( localRepository, remoteArtifactRepositories, new DecorationModel() );
}
- /** {@inheritDoc} */
+ /**
+ * This method is not implemented according to the URI specification and has many weird
+ * corner cases where it doesn't do the right thing. Please consider using a better
+ * implemented method from a different library such as org.apache.http.client.utils.URIUtils#resolve.
+ */
+ @Deprecated
public String getRelativePath( String to, String from )
{
checkNotNull( "to", to );
checkNotNull( "from", from );
-
+
+ if ( to.contains( ":" ) && from.contains( ":" ) )
Review comment:
Is that a mistake? For this to fire, both to and from have to have colons and both to and from have to differ in the substring before the colon, in which case it just returns the to string. The case that seems off is
http://www.example.com:99/foo.html
and
http://www.example.com:77/bar.html
But is there a plausible relative URL between those two?
Also, not all URLs have a //.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-doxia-sitetools] michael-o commented on a change in pull
request #10: [DOXIASITETOOLS-109] handle more URI formats
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #10: [DOXIASITETOOLS-109] handle more URI formats
URL: https://github.com/apache/maven-doxia-sitetools/pull/10#discussion_r381526882
##########
File path: doxia-integration-tools/src/test/java/org/apache/maven/doxia/tools/DefaultSiteToolTest.java
##########
@@ -56,4 +72,45 @@ public void testGetNormalizedPath()
assertEquals( "file:/Documents and Settings/",
DefaultSiteTool.getNormalizedPath( "file://Documents and Settings/" ) );
}
+
+ @SuppressWarnings("deprecation")
+ @Test
+ public void testGetRelativePath()
+ {
+ assertEquals(
+ ".." + File.separator + "bar.html",
+ tool.getRelativePath("http://example.com/foo/bar.html", "http://example.com/foo/baz.html"));
+ }
+
+ @SuppressWarnings("deprecation")
+ @Test
+ public void testGetRelativePath_same()
+ {
+ assertTrue(
+ tool.getRelativePath( "http://example.com/foo/bar.html", "http://example.com/foo/bar.html" ).isEmpty() );
+ }
+
+ @SuppressWarnings("deprecation")
+ @Test
+ public void testGetRelativePath_differentSchemes() {
+ assertEquals(
+ "scp://example.com/foo/bar.html",
+ tool.getRelativePath( "scp://example.com/foo/bar.html", "http://example.com/foo/bar.html" ) );
+ assertEquals(
+ "file:///tmp/bloop",
+ tool.getRelativePath( "file:///tmp/bloop", "scp://localhost:/tmp/blop" ) );
Review comment:
Is omitting the port a valid URI at all?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-doxia-sitetools] michael-o commented on a change in pull
request #10: [DOXIASITETOOLS-109] handle more URI formats
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #10: [DOXIASITETOOLS-109] handle more URI formats
URL: https://github.com/apache/maven-doxia-sitetools/pull/10#discussion_r381526510
##########
File path: doxia-integration-tools/src/main/java/org/apache/maven/doxia/tools/DefaultSiteTool.java
##########
@@ -174,20 +173,34 @@ public Artifact getSkinArtifactFromRepository( ArtifactRepository localRepositor
return artifact;
}
- /** {@inheritDoc} */
public Artifact getDefaultSkinArtifact( ArtifactRepository localRepository,
List<ArtifactRepository> remoteArtifactRepositories )
throws SiteToolException
{
return getSkinArtifactFromRepository( localRepository, remoteArtifactRepositories, new DecorationModel() );
}
- /** {@inheritDoc} */
+ /**
+ * This method is not implemented according to the URI specification and has many weird
+ * corner cases where it doesn't do the right thing. Please consider using a better
+ * implemented method from a different library such as org.apache.http.client.utils.URIUtils#resolve.
+ */
+ @Deprecated
public String getRelativePath( String to, String from )
{
checkNotNull( "to", to );
checkNotNull( "from", from );
-
+
+ if ( to.contains( ":" ) && from.contains( ":" ) )
Review comment:
This could be dangerous/wrong. Consider this: `blabla.com:8080/sdfsd`. No scheme, but you'd still process here. Better check for `://`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-doxia-sitetools] michael-o commented on a change in pull
request #10: [DOXIASITETOOLS-109] handle more URI formats
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #10: [DOXIASITETOOLS-109] handle more URI formats
URL: https://github.com/apache/maven-doxia-sitetools/pull/10#discussion_r381550318
##########
File path: doxia-integration-tools/src/main/java/org/apache/maven/doxia/tools/DefaultSiteTool.java
##########
@@ -174,20 +173,34 @@ public Artifact getSkinArtifactFromRepository( ArtifactRepository localRepositor
return artifact;
}
- /** {@inheritDoc} */
public Artifact getDefaultSkinArtifact( ArtifactRepository localRepository,
List<ArtifactRepository> remoteArtifactRepositories )
throws SiteToolException
{
return getSkinArtifactFromRepository( localRepository, remoteArtifactRepositories, new DecorationModel() );
}
- /** {@inheritDoc} */
+ /**
+ * This method is not implemented according to the URI specification and has many weird
+ * corner cases where it doesn't do the right thing. Please consider using a better
+ * implemented method from a different library such as org.apache.http.client.utils.URIUtils#resolve.
+ */
+ @Deprecated
public String getRelativePath( String to, String from )
{
checkNotNull( "to", to );
checkNotNull( "from", from );
-
+
+ if ( to.contains( ":" ) && from.contains( ":" ) )
Review comment:
Right, `mailto` does not. But Using `contains` paired with `lastIndexOf` would mean with a non-standard port that you substring scheme and authority, don't you?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-doxia-sitetools] elharo merged pull request #10:
[DOXIASITETOOLS-109] handle more URI formats
Posted by GitBox <gi...@apache.org>.
elharo merged pull request #10: [DOXIASITETOOLS-109] handle more URI formats
URL: https://github.com/apache/maven-doxia-sitetools/pull/10
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-doxia-sitetools] michael-o commented on issue #10:
[DOXIASITETOOLS-109] handle more URI formats
Posted by GitBox <gi...@apache.org>.
michael-o commented on issue #10: [DOXIASITETOOLS-109] handle more URI formats
URL: https://github.com/apache/maven-doxia-sitetools/pull/10#issuecomment-593573774
Nothing from the top of my head. I need to think about it a couple of days. Just go ahead and merge. If I can make up something I will open another JIRA issue.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [maven-doxia-sitetools] elharo commented on a change in pull
request #10: [DOXIASITETOOLS-109] handle more URI formats
Posted by GitBox <gi...@apache.org>.
elharo commented on a change in pull request #10: [DOXIASITETOOLS-109] handle more URI formats
URL: https://github.com/apache/maven-doxia-sitetools/pull/10#discussion_r386107619
##########
File path: doxia-integration-tools/src/main/java/org/apache/maven/doxia/tools/DefaultSiteTool.java
##########
@@ -174,20 +173,34 @@ public Artifact getSkinArtifactFromRepository( ArtifactRepository localRepositor
return artifact;
}
- /** {@inheritDoc} */
public Artifact getDefaultSkinArtifact( ArtifactRepository localRepository,
List<ArtifactRepository> remoteArtifactRepositories )
throws SiteToolException
{
return getSkinArtifactFromRepository( localRepository, remoteArtifactRepositories, new DecorationModel() );
}
- /** {@inheritDoc} */
+ /**
+ * This method is not implemented according to the URI specification and has many weird
+ * corner cases where it doesn't do the right thing. Please consider using a better
+ * implemented method from a different library such as org.apache.http.client.utils.URIUtils#resolve.
+ */
+ @Deprecated
public String getRelativePath( String to, String from )
{
checkNotNull( "to", to );
checkNotNull( "from", from );
-
+
+ if ( to.contains( ":" ) && from.contains( ":" ) )
Review comment:
Yes, which is the intended behavior because if they're not the same we can't compute a relative URL.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services