You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cxf.apache.org by GitBox <gi...@apache.org> on 2020/01/24 12:03:21 UTC
[GitHub] [cxf] jgallimore opened a new pull request #630: CXF-8201 add
additional check for file scheme in setUriParts
jgallimore opened a new pull request #630: CXF-8201 add additional check for file scheme in setUriParts
URL: https://github.com/apache/cxf/pull/630
This additional check enables path appends for file URIs to work as it has done in previous versions of CXF, for example, with this patch:
UriBuilder.fromUri("file:///~/calendar").path("extra").build() will return file:///~calendar/extra as opposed to file:///~/calendar.
This has been discussed on the mailing list here: https://mail-archives.apache.org/mod_mbox/cxf-dev/202001.mbox/%3cCAGRgoZhuKXoYW_JtxyDH=u7Qqqcbeo6-9BXa0dxB8L+iGB4J4g@mail.gmail.com%3e
----------------------------------------------------------------
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] [cxf] reta commented on a change in pull request #630: CXF-8201 add
additional check for file scheme in setUriParts
Posted by GitBox <gi...@apache.org>.
reta commented on a change in pull request #630: CXF-8201 add additional check for file scheme in setUriParts
URL: https://github.com/apache/cxf/pull/630#discussion_r370911966
##########
File path: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/UriBuilderImpl.java
##########
@@ -651,8 +651,8 @@ private void setUriParts(URI uri) {
} else {
schemeSpecificPart = uri.getSchemeSpecificPart();
}
- if (scheme != null && host == null && (query == null || query.isEmpty()) && userInfo == null
- && uri.getSchemeSpecificPart() != null) {
+ if (scheme != null && !"file".equalsIgnoreCase(scheme) && host == null && (query == null || query.isEmpty())
Review comment:
Hi @jgallimore, thanks for the PR. I think CXF should not assume any intrinsic knowledge about the URI scheme and handle it in a specific way. For example, what about handling `jar:file`? Or any other custom schemes?
Although the reply on spec list (https://www.eclipse.org/lists/jaxrs-dev/msg00774.html) explicitly mentions that JAX-RS context is REST-over-HTTP (the CXF case), we could certainly make `UriBuilder` more flexible if the general solution could be found.
Hope it makes sense, thank 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] [cxf] jgallimore commented on issue #630: CXF-8201 add additional
check for file scheme in setUriParts
Posted by GitBox <gi...@apache.org>.
jgallimore commented on issue #630: CXF-8201 add additional check for file scheme in setUriParts
URL: https://github.com/apache/cxf/pull/630#issuecomment-578355918
retest this please
----------------------------------------------------------------
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] [cxf] reta commented on a change in pull request #630: CXF-8201 add
additional check for file scheme in setUriParts
Posted by GitBox <gi...@apache.org>.
reta commented on a change in pull request #630: CXF-8201 add additional check for file scheme in setUriParts
URL: https://github.com/apache/cxf/pull/630#discussion_r371571834
##########
File path: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/UriBuilderImpl.java
##########
@@ -661,6 +662,14 @@ private void setUriParts(URI uri) {
}
}
+ private boolean queryIsEmpty() {
+ return query == null || query.isEmpty();
Review comment:
We have a helper method for it, `CollectionUtils.isEmpty(query)` from `org.apache.cxf.common.util`
----------------------------------------------------------------
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] [cxf] reta commented on a change in pull request #630: CXF-8201 add
additional check for file scheme in setUriParts
Posted by GitBox <gi...@apache.org>.
reta commented on a change in pull request #630: CXF-8201 add additional check for file scheme in setUriParts
URL: https://github.com/apache/cxf/pull/630#discussion_r371596346
##########
File path: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/UriBuilderImpl.java
##########
@@ -661,6 +662,14 @@ private void setUriParts(URI uri) {
}
}
+ private boolean queryIsEmpty() {
+ return query == null || query.isEmpty();
+ }
+
+ private boolean sspMatchessPath(final URI uri) {
Review comment:
`Matchess` is misspelled, `schemeSpecificPartMatchesUriPath` would be probably better name (despite being long).
----------------------------------------------------------------
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] [cxf] reta merged pull request #630: CXF-8201 add additional check
for file scheme in setUriParts
Posted by GitBox <gi...@apache.org>.
reta merged pull request #630: CXF-8201 add additional check for file scheme in setUriParts
URL: https://github.com/apache/cxf/pull/630
----------------------------------------------------------------
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] [cxf] reta commented on a change in pull request #630: CXF-8201 add
additional check for file scheme in setUriParts
Posted by GitBox <gi...@apache.org>.
reta commented on a change in pull request #630: CXF-8201 add additional check for file scheme in setUriParts
URL: https://github.com/apache/cxf/pull/630#discussion_r371596198
##########
File path: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/UriBuilderImpl.java
##########
@@ -661,6 +662,14 @@ private void setUriParts(URI uri) {
}
}
+ private boolean queryIsEmpty() {
+ return query == null || query.isEmpty();
+ }
+
+ private boolean sspMatchessPath(final URI uri) {
+ return uri.getRawSchemeSpecificPart() != null && uri.getRawSchemeSpecificPart().equals("//" + uri.getPath());
Review comment:
It would be good to check if `uri.getPath()` is not null, otherwise `"//" + uri.getPath()` becomes something like `//null`
----------------------------------------------------------------
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] [cxf] reta commented on issue #630: CXF-8201 add additional check
for file scheme in setUriParts
Posted by GitBox <gi...@apache.org>.
reta commented on issue #630: CXF-8201 add additional check for file scheme in setUriParts
URL: https://github.com/apache/cxf/pull/630#issuecomment-580037346
@jgallimore thank 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] [cxf] reta commented on issue #630: CXF-8201 add additional check
for file scheme in setUriParts
Posted by GitBox <gi...@apache.org>.
reta commented on issue #630: CXF-8201 add additional check for file scheme in setUriParts
URL: https://github.com/apache/cxf/pull/630#issuecomment-579064666
@jgallimore thanks, I think we can get it in like that, a few comments please to address, thank 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] [cxf] jgallimore commented on issue #630: CXF-8201 add additional
check for file scheme in setUriParts
Posted by GitBox <gi...@apache.org>.
jgallimore commented on issue #630: CXF-8201 add additional check for file scheme in setUriParts
URL: https://github.com/apache/cxf/pull/630#issuecomment-578752282
@reta Thanks for the feedback. The conversation continues on the spec list - there's a note about UriBuilder referencing rfc-3986 - https://tools.ietf.org/html/rfc3986. I've pushed a further commit to try and make this more generic by checking the path and raw scheme specific element.
----------------------------------------------------------------
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] [cxf] jgallimore commented on issue #630: CXF-8201 add additional
check for file scheme in setUriParts
Posted by GitBox <gi...@apache.org>.
jgallimore commented on issue #630: CXF-8201 add additional check for file scheme in setUriParts
URL: https://github.com/apache/cxf/pull/630#issuecomment-579739570
@reta Thank you again for the feedback. I've hopefully addressed those items!
----------------------------------------------------------------
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