You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cxf.apache.org by Jonathan Gallimore <jo...@gmail.com> on 2020/01/23 20:59:37 UTC
UriBuilder issue
Hi
I'm looking into an issue where appending a path to file:// URI with
UriBuilder doesn't work - this has been flagged up in TomEE 8.0.1 which is
using CXF 3.3.4, but works ok with TomEE 7.1.1 (which uses CXF 3.1.18).
In essence, this fails:
UriBuilder.fromUri("file:///~/calendar").path("extra").build().toString() -
the result is file:///~/calendar. without the "/extra" on the end. This is
because the schemeSpecificPart field is set in this if block:
https://github.com/apache/cxf/blob/e6ec5b785e67bbd2464796119a9cdcacf59598c8/rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/UriBuilderImpl.java#L642-L645
and
is used when building the URI, so the appended path is lost.
I've attempted to patch this, by adding a check for a path as well as a
query in this if block:
diff --git
a/rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/UriBuilderImpl.java
b/rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/UriBuilderImpl.java
index 8de34fbb0d..26e7ca357d 100644
---
a/rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/UriBuilderImpl.java
+++
b/rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/UriBuilderImpl.java
@@ -651,8 +651,9 @@ public class UriBuilderImpl extends UriBuilder
implements Cloneable {
} else {
schemeSpecificPart = uri.getSchemeSpecificPart();
}
- if (scheme != null && host == null && (query == null ||
query.isEmpty()) && userInfo == null
- && uri.getSchemeSpecificPart() != null) {
+ if (scheme != null && host == null
+ && queryEmpty() && pathEmpty() && userInfo == null
+ && uri.getSchemeSpecificPart() != null) {
schemeSpecificPart = uri.getSchemeSpecificPart();
}
String theFragment = uri.getFragment();
@@ -661,6 +662,14 @@ public class UriBuilderImpl extends UriBuilder
implements Cloneable {
}
}
+ private boolean queryEmpty() {
+ return query == null || query.isEmpty();
+ }
+
+ private boolean pathEmpty() {
+ return paths == null || paths.isEmpty();
+ }
+
This fixes the issue with the file URI above, but causes this test failure:
[ERROR] UriBuilderImplTest.testURItoStringMatchesOriginalURI:1631
expected:<myscheme://[not.really.a.host:fakePort]/> but was:<myscheme://[]/>
I note that this check was added in this commit:
https://github.com/apache/cxf/commit/e6ec5b785e67bbd2464796119a9cdcacf59598c8,
and that references some TCK tests. Is the fakePort scenario necessary to
pass the TCK, or is there other rationale for that case?
I'm very happy to work on a PR to fix along with tests, but would
appreciate a little guidance if there's a specific approach to take. I'll
keep looking to find a way to handle the failing assert I mention above,
but if I've missed something blindingly obvious, please do let me know!
Many thanks
Jon
Re: UriBuilder issue
Posted by Jonathan Gallimore <jo...@gmail.com>.
I've created a PR for the file check: https://github.com/apache/cxf/pull/630
Many thanks
Jon
On Fri, Jan 24, 2020 at 9:58 AM Jonathan Gallimore <
jonathan.gallimore@gmail.com> wrote:
> Hi Romain
>
> Thanks for the reply. That sounds reasonable - I'll raise it on the list
> for the spec to seek clarification, and add assertions to the TCK if
> appropriate. I had considered a check for "file" as the scheme (in fact
> that was my first attempt :-) ), but was worried it maybe wasn't generic
> enough. I can certainly submit a PR with that change and a test case.
>
> Cheers
>
> Jon
>
> On Thu, Jan 23, 2020 at 9:47 PM Romain Manni-Bucau <rm...@gmail.com>
> wrote:
>
>> Hi Jon
>>
>> Sounds like the fix respects the spec now,
>>
>> https://docs.oracle.com/javaee/7/api/javax/ws/rs/core/UriBuilder.html#schemeSpecificPart-java.lang.String-
>> implies there is no path so
>>
>> https://docs.oracle.com/javaee/7/api/javax/ws/rs/core/UriBuilder.html#path-java.lang.String-
>> is a noop even if unnatural.
>>
>> Can request a spec clarification and tck challenge IMHO to let cxf fix the
>> buildUriString and setUriParts methods.
>>
>> A likely trivial way to fix is to test file:// protocol, tck will pass and
>> behavior will be more natural IMHO.
>>
>> Le jeu. 23 janv. 2020 à 21:59, Jonathan Gallimore <
>> jonathan.gallimore@gmail.com> a écrit :
>>
>> > Hi
>> >
>> > I'm looking into an issue where appending a path to file:// URI with
>> > UriBuilder doesn't work - this has been flagged up in TomEE 8.0.1 which
>> is
>> > using CXF 3.3.4, but works ok with TomEE 7.1.1 (which uses CXF 3.1.18).
>> >
>> > In essence, this fails:
>> >
>> >
>> UriBuilder.fromUri("file:///~/calendar").path("extra").build().toString() -
>> > the result is file:///~/calendar. without the "/extra" on the end. This
>> is
>> > because the schemeSpecificPart field is set in this if block:
>> >
>> >
>> https://github.com/apache/cxf/blob/e6ec5b785e67bbd2464796119a9cdcacf59598c8/rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/UriBuilderImpl.java#L642-L645
>> > and
>> > is used when building the URI, so the appended path is lost.
>> >
>> > I've attempted to patch this, by adding a check for a path as well as a
>> > query in this if block:
>> >
>> > diff --git
>> >
>> >
>> a/rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/UriBuilderImpl.java
>> >
>> >
>> b/rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/UriBuilderImpl.java
>> > index 8de34fbb0d..26e7ca357d 100644
>> > ---
>> >
>> >
>> a/rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/UriBuilderImpl.java
>> > +++
>> >
>> >
>> b/rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/UriBuilderImpl.java
>> > @@ -651,8 +651,9 @@ public class UriBuilderImpl extends UriBuilder
>> > implements Cloneable {
>> > } else {
>> > schemeSpecificPart = uri.getSchemeSpecificPart();
>> > }
>> > - if (scheme != null && host == null && (query == null ||
>> > query.isEmpty()) && userInfo == null
>> > - && uri.getSchemeSpecificPart() != null) {
>> > + if (scheme != null && host == null
>> > + && queryEmpty() && pathEmpty() && userInfo == null
>> > + && uri.getSchemeSpecificPart() != null) {
>> > schemeSpecificPart = uri.getSchemeSpecificPart();
>> > }
>> > String theFragment = uri.getFragment();
>> > @@ -661,6 +662,14 @@ public class UriBuilderImpl extends UriBuilder
>> > implements Cloneable {
>> > }
>> > }
>> >
>> > + private boolean queryEmpty() {
>> > + return query == null || query.isEmpty();
>> > + }
>> > +
>> > + private boolean pathEmpty() {
>> > + return paths == null || paths.isEmpty();
>> > + }
>> > +
>> >
>> > This fixes the issue with the file URI above, but causes this test
>> failure:
>> >
>> > [ERROR] UriBuilderImplTest.testURItoStringMatchesOriginalURI:1631
>> > expected:<myscheme://[not.really.a.host:fakePort]/> but
>> > was:<myscheme://[]/>
>> >
>> > I note that this check was added in this commit:
>> >
>> >
>> https://github.com/apache/cxf/commit/e6ec5b785e67bbd2464796119a9cdcacf59598c8
>> > ,
>> > and that references some TCK tests. Is the fakePort scenario necessary
>> to
>> > pass the TCK, or is there other rationale for that case?
>> >
>> > I'm very happy to work on a PR to fix along with tests, but would
>> > appreciate a little guidance if there's a specific approach to take.
>> I'll
>> > keep looking to find a way to handle the failing assert I mention above,
>> > but if I've missed something blindingly obvious, please do let me know!
>> >
>> > Many thanks
>> >
>> > Jon
>> >
>>
>
Re: UriBuilder issue
Posted by Jonathan Gallimore <jo...@gmail.com>.
FYI - I've sent over a request for clarification on the spec:
https://www.eclipse.org/lists/jaxrs-dev/msg00773.html
Jon
On Fri, Jan 24, 2020 at 9:58 AM Jonathan Gallimore <
jonathan.gallimore@gmail.com> wrote:
> Hi Romain
>
> Thanks for the reply. That sounds reasonable - I'll raise it on the list
> for the spec to seek clarification, and add assertions to the TCK if
> appropriate. I had considered a check for "file" as the scheme (in fact
> that was my first attempt :-) ), but was worried it maybe wasn't generic
> enough. I can certainly submit a PR with that change and a test case.
>
> Cheers
>
> Jon
>
> On Thu, Jan 23, 2020 at 9:47 PM Romain Manni-Bucau <rm...@gmail.com>
> wrote:
>
>> Hi Jon
>>
>> Sounds like the fix respects the spec now,
>>
>> https://docs.oracle.com/javaee/7/api/javax/ws/rs/core/UriBuilder.html#schemeSpecificPart-java.lang.String-
>> implies there is no path so
>>
>> https://docs.oracle.com/javaee/7/api/javax/ws/rs/core/UriBuilder.html#path-java.lang.String-
>> is a noop even if unnatural.
>>
>> Can request a spec clarification and tck challenge IMHO to let cxf fix the
>> buildUriString and setUriParts methods.
>>
>> A likely trivial way to fix is to test file:// protocol, tck will pass and
>> behavior will be more natural IMHO.
>>
>> Le jeu. 23 janv. 2020 à 21:59, Jonathan Gallimore <
>> jonathan.gallimore@gmail.com> a écrit :
>>
>> > Hi
>> >
>> > I'm looking into an issue where appending a path to file:// URI with
>> > UriBuilder doesn't work - this has been flagged up in TomEE 8.0.1 which
>> is
>> > using CXF 3.3.4, but works ok with TomEE 7.1.1 (which uses CXF 3.1.18).
>> >
>> > In essence, this fails:
>> >
>> >
>> UriBuilder.fromUri("file:///~/calendar").path("extra").build().toString() -
>> > the result is file:///~/calendar. without the "/extra" on the end. This
>> is
>> > because the schemeSpecificPart field is set in this if block:
>> >
>> >
>> https://github.com/apache/cxf/blob/e6ec5b785e67bbd2464796119a9cdcacf59598c8/rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/UriBuilderImpl.java#L642-L645
>> > and
>> > is used when building the URI, so the appended path is lost.
>> >
>> > I've attempted to patch this, by adding a check for a path as well as a
>> > query in this if block:
>> >
>> > diff --git
>> >
>> >
>> a/rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/UriBuilderImpl.java
>> >
>> >
>> b/rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/UriBuilderImpl.java
>> > index 8de34fbb0d..26e7ca357d 100644
>> > ---
>> >
>> >
>> a/rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/UriBuilderImpl.java
>> > +++
>> >
>> >
>> b/rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/UriBuilderImpl.java
>> > @@ -651,8 +651,9 @@ public class UriBuilderImpl extends UriBuilder
>> > implements Cloneable {
>> > } else {
>> > schemeSpecificPart = uri.getSchemeSpecificPart();
>> > }
>> > - if (scheme != null && host == null && (query == null ||
>> > query.isEmpty()) && userInfo == null
>> > - && uri.getSchemeSpecificPart() != null) {
>> > + if (scheme != null && host == null
>> > + && queryEmpty() && pathEmpty() && userInfo == null
>> > + && uri.getSchemeSpecificPart() != null) {
>> > schemeSpecificPart = uri.getSchemeSpecificPart();
>> > }
>> > String theFragment = uri.getFragment();
>> > @@ -661,6 +662,14 @@ public class UriBuilderImpl extends UriBuilder
>> > implements Cloneable {
>> > }
>> > }
>> >
>> > + private boolean queryEmpty() {
>> > + return query == null || query.isEmpty();
>> > + }
>> > +
>> > + private boolean pathEmpty() {
>> > + return paths == null || paths.isEmpty();
>> > + }
>> > +
>> >
>> > This fixes the issue with the file URI above, but causes this test
>> failure:
>> >
>> > [ERROR] UriBuilderImplTest.testURItoStringMatchesOriginalURI:1631
>> > expected:<myscheme://[not.really.a.host:fakePort]/> but
>> > was:<myscheme://[]/>
>> >
>> > I note that this check was added in this commit:
>> >
>> >
>> https://github.com/apache/cxf/commit/e6ec5b785e67bbd2464796119a9cdcacf59598c8
>> > ,
>> > and that references some TCK tests. Is the fakePort scenario necessary
>> to
>> > pass the TCK, or is there other rationale for that case?
>> >
>> > I'm very happy to work on a PR to fix along with tests, but would
>> > appreciate a little guidance if there's a specific approach to take.
>> I'll
>> > keep looking to find a way to handle the failing assert I mention above,
>> > but if I've missed something blindingly obvious, please do let me know!
>> >
>> > Many thanks
>> >
>> > Jon
>> >
>>
>
Re: UriBuilder issue
Posted by Jonathan Gallimore <jo...@gmail.com>.
Hi Romain
Thanks for the reply. That sounds reasonable - I'll raise it on the list
for the spec to seek clarification, and add assertions to the TCK if
appropriate. I had considered a check for "file" as the scheme (in fact
that was my first attempt :-) ), but was worried it maybe wasn't generic
enough. I can certainly submit a PR with that change and a test case.
Cheers
Jon
On Thu, Jan 23, 2020 at 9:47 PM Romain Manni-Bucau <rm...@gmail.com>
wrote:
> Hi Jon
>
> Sounds like the fix respects the spec now,
>
> https://docs.oracle.com/javaee/7/api/javax/ws/rs/core/UriBuilder.html#schemeSpecificPart-java.lang.String-
> implies there is no path so
>
> https://docs.oracle.com/javaee/7/api/javax/ws/rs/core/UriBuilder.html#path-java.lang.String-
> is a noop even if unnatural.
>
> Can request a spec clarification and tck challenge IMHO to let cxf fix the
> buildUriString and setUriParts methods.
>
> A likely trivial way to fix is to test file:// protocol, tck will pass and
> behavior will be more natural IMHO.
>
> Le jeu. 23 janv. 2020 à 21:59, Jonathan Gallimore <
> jonathan.gallimore@gmail.com> a écrit :
>
> > Hi
> >
> > I'm looking into an issue where appending a path to file:// URI with
> > UriBuilder doesn't work - this has been flagged up in TomEE 8.0.1 which
> is
> > using CXF 3.3.4, but works ok with TomEE 7.1.1 (which uses CXF 3.1.18).
> >
> > In essence, this fails:
> >
> >
> UriBuilder.fromUri("file:///~/calendar").path("extra").build().toString() -
> > the result is file:///~/calendar. without the "/extra" on the end. This
> is
> > because the schemeSpecificPart field is set in this if block:
> >
> >
> https://github.com/apache/cxf/blob/e6ec5b785e67bbd2464796119a9cdcacf59598c8/rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/UriBuilderImpl.java#L642-L645
> > and
> > is used when building the URI, so the appended path is lost.
> >
> > I've attempted to patch this, by adding a check for a path as well as a
> > query in this if block:
> >
> > diff --git
> >
> >
> a/rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/UriBuilderImpl.java
> >
> >
> b/rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/UriBuilderImpl.java
> > index 8de34fbb0d..26e7ca357d 100644
> > ---
> >
> >
> a/rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/UriBuilderImpl.java
> > +++
> >
> >
> b/rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/UriBuilderImpl.java
> > @@ -651,8 +651,9 @@ public class UriBuilderImpl extends UriBuilder
> > implements Cloneable {
> > } else {
> > schemeSpecificPart = uri.getSchemeSpecificPart();
> > }
> > - if (scheme != null && host == null && (query == null ||
> > query.isEmpty()) && userInfo == null
> > - && uri.getSchemeSpecificPart() != null) {
> > + if (scheme != null && host == null
> > + && queryEmpty() && pathEmpty() && userInfo == null
> > + && uri.getSchemeSpecificPart() != null) {
> > schemeSpecificPart = uri.getSchemeSpecificPart();
> > }
> > String theFragment = uri.getFragment();
> > @@ -661,6 +662,14 @@ public class UriBuilderImpl extends UriBuilder
> > implements Cloneable {
> > }
> > }
> >
> > + private boolean queryEmpty() {
> > + return query == null || query.isEmpty();
> > + }
> > +
> > + private boolean pathEmpty() {
> > + return paths == null || paths.isEmpty();
> > + }
> > +
> >
> > This fixes the issue with the file URI above, but causes this test
> failure:
> >
> > [ERROR] UriBuilderImplTest.testURItoStringMatchesOriginalURI:1631
> > expected:<myscheme://[not.really.a.host:fakePort]/> but
> > was:<myscheme://[]/>
> >
> > I note that this check was added in this commit:
> >
> >
> https://github.com/apache/cxf/commit/e6ec5b785e67bbd2464796119a9cdcacf59598c8
> > ,
> > and that references some TCK tests. Is the fakePort scenario necessary to
> > pass the TCK, or is there other rationale for that case?
> >
> > I'm very happy to work on a PR to fix along with tests, but would
> > appreciate a little guidance if there's a specific approach to take. I'll
> > keep looking to find a way to handle the failing assert I mention above,
> > but if I've missed something blindingly obvious, please do let me know!
> >
> > Many thanks
> >
> > Jon
> >
>
Re: UriBuilder issue
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Hi Jon
Sounds like the fix respects the spec now,
https://docs.oracle.com/javaee/7/api/javax/ws/rs/core/UriBuilder.html#schemeSpecificPart-java.lang.String-
implies there is no path so
https://docs.oracle.com/javaee/7/api/javax/ws/rs/core/UriBuilder.html#path-java.lang.String-
is a noop even if unnatural.
Can request a spec clarification and tck challenge IMHO to let cxf fix the
buildUriString and setUriParts methods.
A likely trivial way to fix is to test file:// protocol, tck will pass and
behavior will be more natural IMHO.
Le jeu. 23 janv. 2020 à 21:59, Jonathan Gallimore <
jonathan.gallimore@gmail.com> a écrit :
> Hi
>
> I'm looking into an issue where appending a path to file:// URI with
> UriBuilder doesn't work - this has been flagged up in TomEE 8.0.1 which is
> using CXF 3.3.4, but works ok with TomEE 7.1.1 (which uses CXF 3.1.18).
>
> In essence, this fails:
>
> UriBuilder.fromUri("file:///~/calendar").path("extra").build().toString() -
> the result is file:///~/calendar. without the "/extra" on the end. This is
> because the schemeSpecificPart field is set in this if block:
>
> https://github.com/apache/cxf/blob/e6ec5b785e67bbd2464796119a9cdcacf59598c8/rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/UriBuilderImpl.java#L642-L645
> and
> is used when building the URI, so the appended path is lost.
>
> I've attempted to patch this, by adding a check for a path as well as a
> query in this if block:
>
> diff --git
>
> a/rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/UriBuilderImpl.java
>
> b/rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/UriBuilderImpl.java
> index 8de34fbb0d..26e7ca357d 100644
> ---
>
> a/rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/UriBuilderImpl.java
> +++
>
> b/rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/UriBuilderImpl.java
> @@ -651,8 +651,9 @@ public class UriBuilderImpl extends UriBuilder
> implements Cloneable {
> } else {
> schemeSpecificPart = uri.getSchemeSpecificPart();
> }
> - if (scheme != null && host == null && (query == null ||
> query.isEmpty()) && userInfo == null
> - && uri.getSchemeSpecificPart() != null) {
> + if (scheme != null && host == null
> + && queryEmpty() && pathEmpty() && userInfo == null
> + && uri.getSchemeSpecificPart() != null) {
> schemeSpecificPart = uri.getSchemeSpecificPart();
> }
> String theFragment = uri.getFragment();
> @@ -661,6 +662,14 @@ public class UriBuilderImpl extends UriBuilder
> implements Cloneable {
> }
> }
>
> + private boolean queryEmpty() {
> + return query == null || query.isEmpty();
> + }
> +
> + private boolean pathEmpty() {
> + return paths == null || paths.isEmpty();
> + }
> +
>
> This fixes the issue with the file URI above, but causes this test failure:
>
> [ERROR] UriBuilderImplTest.testURItoStringMatchesOriginalURI:1631
> expected:<myscheme://[not.really.a.host:fakePort]/> but
> was:<myscheme://[]/>
>
> I note that this check was added in this commit:
>
> https://github.com/apache/cxf/commit/e6ec5b785e67bbd2464796119a9cdcacf59598c8
> ,
> and that references some TCK tests. Is the fakePort scenario necessary to
> pass the TCK, or is there other rationale for that case?
>
> I'm very happy to work on a PR to fix along with tests, but would
> appreciate a little guidance if there's a specific approach to take. I'll
> keep looking to find a way to handle the failing assert I mention above,
> but if I've missed something blindingly obvious, please do let me know!
>
> Many thanks
>
> Jon
>