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/09/22 18:29:36 UTC
[GitHub] [cxf] andymc12 opened a new pull request #697: [CXF-8346] TCK Changes for URI and hasEntity checks
andymc12 opened a new pull request #697:
URL: https://github.com/apache/cxf/pull/697
Fixes various "hasEntity" TCK tests where the entityStream is non-null, but empty. Also fixes an issue with `ContainerRequestContext.setRequestUri(...)` methods.
Resolves [CXF-8346](https://issues.apache.org/jira/browse/CXF-8346).
----------------------------------------------------------------
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
[GitHub] [cxf] reta commented on a change in pull request #697: [CXF-8346] TCK Changes for URI and hasEntity checks
Posted by GitBox <gi...@apache.org>.
reta commented on a change in pull request #697:
URL: https://github.com/apache/cxf/pull/697#discussion_r493106096
##########
File path: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/ResponseImpl.java
##########
@@ -137,22 +143,70 @@ public Object getActualEntity() {
return lastEntity != null ? lastEntity : entity;
}
+ @Override
public Object getEntity() {
return InjectionUtils.getEntity(getActualEntity());
}
+ @Override
public boolean hasEntity() {
- return getActualEntity() != null;
+ // per spec, need to check if the stream exists and if it has data.
+ Object actualEntity = getActualEntity();
+ if (actualEntity == null) {
+ return false;
+ } else if (actualEntity instanceof InputStream) {
+ final InputStream is = (InputStream) actualEntity;
+ try {
+ if (is.markSupported()) {
+ is.mark(1);
+ int i = is.read();
+ is.reset();
+ return i != -1;
+ } else {
+ try {
+ if (is.available() > 0) {
+ return true;
+ }
+ } catch (IOException ioe) {
+ //Do nothing
+ }
+ int b = is.read();
Review comment:
This would modify the state of the stream, correct? May be it would be simpler to check/construct the PushbackInputStream and than do read / unread?
----------------------------------------------------------------
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
[GitHub] [cxf] reta commented on a change in pull request #697: [CXF-8346] TCK Changes for URI and hasEntity checks
Posted by GitBox <gi...@apache.org>.
reta commented on a change in pull request #697:
URL: https://github.com/apache/cxf/pull/697#discussion_r493108076
##########
File path: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/ResponseImpl.java
##########
@@ -137,22 +143,70 @@ public Object getActualEntity() {
return lastEntity != null ? lastEntity : entity;
}
+ @Override
public Object getEntity() {
return InjectionUtils.getEntity(getActualEntity());
}
+ @Override
public boolean hasEntity() {
- return getActualEntity() != null;
+ // per spec, need to check if the stream exists and if it has data.
+ Object actualEntity = getActualEntity();
+ if (actualEntity == null) {
+ return false;
+ } else if (actualEntity instanceof InputStream) {
+ final InputStream is = (InputStream) actualEntity;
+ try {
+ if (is.markSupported()) {
+ is.mark(1);
+ int i = is.read();
+ is.reset();
+ return i != -1;
+ } else {
+ try {
+ if (is.available() > 0) {
+ return true;
+ }
+ } catch (IOException ioe) {
+ //Do nothing
+ }
+ int b = is.read();
+ if (b == -1) {
+ return false;
+ }
+ PushbackInputStream pbis;
+ if (is instanceof PushbackInputStream) {
+ pbis = (PushbackInputStream) is;
+ } else {
+ pbis = new PushbackInputStream(is, 1);
+ if (lastEntity != null) {
Review comment:
Unfamiliar with this part, do you have insights about `lastEntity` vs `entity`?
----------------------------------------------------------------
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
[GitHub] [cxf] coheigea commented on pull request #697: [CXF-8346] TCK Changes for URI and hasEntity checks
Posted by GitBox <gi...@apache.org>.
coheigea commented on pull request #697:
URL: https://github.com/apache/cxf/pull/697#issuecomment-702553569
Hi @andymc12 ,
This change is causing quite a few system test failures, e.g. https://ci-builds.apache.org/job/CXF/job/CXF-JDK8/85/#showFailuresLink
Could you take a look? Maybe the tests themselves need to be updated.
----------------------------------------------------------------
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
[GitHub] [cxf] andymc12 commented on a change in pull request #697: [CXF-8346] TCK Changes for URI and hasEntity checks
Posted by GitBox <gi...@apache.org>.
andymc12 commented on a change in pull request #697:
URL: https://github.com/apache/cxf/pull/697#discussion_r493884623
##########
File path: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/ResponseImpl.java
##########
@@ -527,10 +621,47 @@ public String getReasonPhrase() {
return statusEnum != null ? statusEnum.getReasonPhrase() : "";
}
+ @Override
public int getStatusCode() {
return statusCode;
}
};
}
-}
+
+ private enum PrimitiveTypes {
+ BYTE(Byte.class, byte.class) { },
+ SHORT(Short.class, short.class) { },
+ INTEGER(Integer.class, int.class) { },
+ LONG(Long.class, long.class) { },
+ FLOAT(Float.class, float.class) { },
+ DOUBLE(Double.class, double.class) { },
+ BOOLEAN(Boolean.class, boolean.class) { },
+ CHAR(Character.class, char.class) { };
+
+ private final Class<?> wrapper;
+ private final Class<?> primitive;
+
+ PrimitiveTypes(Class<?> wrapper, Class<?> primitive) {
+ this.wrapper = wrapper;
+ this.primitive = primitive;
+ }
+
+ public static PrimitiveTypes forType(Class<?> type) {
+ for (PrimitiveTypes primitive : PrimitiveTypes.values()) {
+ if (primitive.supports(type)) {
+ return primitive;
+ }
+ }
+ return null;
+ }
+
+ public boolean supports(Class<?> type) {
+ return type == wrapper || type == primitive;
+ }
+ }
+
+ private static boolean isBasicType(Class<?> type) {
+ return PrimitiveTypes.forType(type) != null || Number.class.isAssignableFrom(type);
Review comment:
Good call - I'm not sure why the PrimitiveTypes inner class was created... my first guess was performance, but it actually looks like it would perform _worse_. I'll use your suggestion.
----------------------------------------------------------------
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
[GitHub] [cxf] andymc12 commented on a change in pull request #697: [CXF-8346] TCK Changes for URI and hasEntity checks
Posted by GitBox <gi...@apache.org>.
andymc12 commented on a change in pull request #697:
URL: https://github.com/apache/cxf/pull/697#discussion_r493882612
##########
File path: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/ResponseImpl.java
##########
@@ -218,42 +274,46 @@ private Date doGetDate(String dateHeader) {
: HttpUtils.getHttpDate(value.toString());
}
+ @Override
public EntityTag getEntityTag() {
Object header = metadata.getFirst(HttpHeaders.ETAG);
return header == null || header instanceof EntityTag ? (EntityTag)header
: EntityTag.valueOf(header.toString());
}
+ @Override
public Locale getLanguage() {
Object header = metadata.getFirst(HttpHeaders.CONTENT_LANGUAGE);
return header == null || header instanceof Locale ? (Locale)header
: HttpUtils.getLocale(header.toString());
}
+ @Override
public Date getLastModified() {
return doGetDate(HttpHeaders.LAST_MODIFIED);
}
+ @Override
public int getLength() {
Object header = metadata.getFirst(HttpHeaders.CONTENT_LENGTH);
return HttpUtils.getContentLength(header == null ? null : header.toString());
}
+ @Override
public URI getLocation() {
Object header = metadata.getFirst(HttpHeaders.LOCATION);
- if (header == null && outMessage != null) {
- header = outMessage.get(Message.REQUEST_URI);
- }
- return header == null || header instanceof URI ? (URI) header
+ return header == null || header instanceof URI ? (URI)header
Review comment:
I believe that was a TCK test. IIRC, the TCK tests that `getLocation` should return null unless explicitly set as a `Location: <someURI>` header (via the `ResponseBuilder.location(URI)` method).
I'm not positive, but I think it addresses this TCK test (currently listed in the failing tests):
com/sun/ts/tests/jaxrs/ee/rs/core/response/JAXRSClient.java#getLocationNotPresentTest_from_standalone: JAXRSClient_getLocationNotPresentTest_from_standalone
----------------------------------------------------------------
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
[GitHub] [cxf] andymc12 commented on a change in pull request #697: [CXF-8346] TCK Changes for URI and hasEntity checks
Posted by GitBox <gi...@apache.org>.
andymc12 commented on a change in pull request #697:
URL: https://github.com/apache/cxf/pull/697#discussion_r493877553
##########
File path: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/ResponseImpl.java
##########
@@ -400,12 +472,30 @@ private Link makeAbsoluteLink(Link link) {
responseMessage.put(Message.PROTOCOL_HEADERS, getHeaders());
lastEntity = JAXRSUtils.readFromMessageBodyReader(readers, cls, t,
- anns,
- entityStream,
- mediaType,
- responseMessage);
- autoClose(cls, false);
- return castLastEntity();
+ anns,
+ entityStream,
+ mediaType,
+ responseMessage);
+ // close the entity after readEntity is called.
+ T tCastLastEntity = castLastEntity();
+ shouldClose = shouldClose && !(tCastLastEntity instanceof Closeable)
Review comment:
`Closeable` extends `AutoCloseable`, so we could get both with by checking `AutoCloseable` - I'll update this.
----------------------------------------------------------------
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
[GitHub] [cxf] reta commented on a change in pull request #697: [CXF-8346] TCK Changes for URI and hasEntity checks
Posted by GitBox <gi...@apache.org>.
reta commented on a change in pull request #697:
URL: https://github.com/apache/cxf/pull/697#discussion_r493110205
##########
File path: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/ResponseImpl.java
##########
@@ -400,12 +472,30 @@ private Link makeAbsoluteLink(Link link) {
responseMessage.put(Message.PROTOCOL_HEADERS, getHeaders());
lastEntity = JAXRSUtils.readFromMessageBodyReader(readers, cls, t,
- anns,
- entityStream,
- mediaType,
- responseMessage);
- autoClose(cls, false);
- return castLastEntity();
+ anns,
+ entityStream,
+ mediaType,
+ responseMessage);
+ // close the entity after readEntity is called.
+ T tCastLastEntity = castLastEntity();
+ shouldClose = shouldClose && !(tCastLastEntity instanceof Closeable)
Review comment:
Shouldn't `AutoCloseable` also be checked?
----------------------------------------------------------------
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
[GitHub] [cxf] andymc12 commented on a change in pull request #697: [CXF-8346] TCK Changes for URI and hasEntity checks
Posted by GitBox <gi...@apache.org>.
andymc12 commented on a change in pull request #697:
URL: https://github.com/apache/cxf/pull/697#discussion_r493915066
##########
File path: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/ResponseImpl.java
##########
@@ -137,22 +143,70 @@ public Object getActualEntity() {
return lastEntity != null ? lastEntity : entity;
}
+ @Override
public Object getEntity() {
return InjectionUtils.getEntity(getActualEntity());
}
+ @Override
public boolean hasEntity() {
- return getActualEntity() != null;
+ // per spec, need to check if the stream exists and if it has data.
+ Object actualEntity = getActualEntity();
+ if (actualEntity == null) {
+ return false;
+ } else if (actualEntity instanceof InputStream) {
+ final InputStream is = (InputStream) actualEntity;
+ try {
+ if (is.markSupported()) {
+ is.mark(1);
+ int i = is.read();
+ is.reset();
+ return i != -1;
+ } else {
+ try {
+ if (is.available() > 0) {
+ return true;
+ }
+ } catch (IOException ioe) {
+ //Do nothing
+ }
+ int b = is.read();
Review comment:
This does read one byte of the stream. If it is -1, then it simply returns false (no entity). If there is data on the stream, then we construct the PushbackInputStream to unread the byte we checked before. I think this approach is optimal for the case where there is no entity, but not harmful in the case where there is. If the stream supports marking (fair uncommon imo) or the available method (much more common), then we'll never get this far.
Unless you object, I'd like to leave it this way, but I can add a unit test to ensure the different expectations and complete coverage (input stream supporting marking, available, etc.).
----------------------------------------------------------------
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
[GitHub] [cxf] reta commented on a change in pull request #697: [CXF-8346] TCK Changes for URI and hasEntity checks
Posted by GitBox <gi...@apache.org>.
reta commented on a change in pull request #697:
URL: https://github.com/apache/cxf/pull/697#discussion_r493960911
##########
File path: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/ResponseImpl.java
##########
@@ -137,22 +143,70 @@ public Object getActualEntity() {
return lastEntity != null ? lastEntity : entity;
}
+ @Override
public Object getEntity() {
return InjectionUtils.getEntity(getActualEntity());
}
+ @Override
public boolean hasEntity() {
- return getActualEntity() != null;
+ // per spec, need to check if the stream exists and if it has data.
+ Object actualEntity = getActualEntity();
+ if (actualEntity == null) {
+ return false;
+ } else if (actualEntity instanceof InputStream) {
+ final InputStream is = (InputStream) actualEntity;
+ try {
+ if (is.markSupported()) {
+ is.mark(1);
+ int i = is.read();
+ is.reset();
+ return i != -1;
+ } else {
+ try {
+ if (is.available() > 0) {
+ return true;
+ }
+ } catch (IOException ioe) {
+ //Do nothing
+ }
+ int b = is.read();
Review comment:
No objections, the reasoning was to keep PushbackInputStream manipulations close by but indeed, you have a good argument in not doing so, 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
[GitHub] [cxf] andymc12 merged pull request #697: [CXF-8346] TCK Changes for URI and hasEntity checks
Posted by GitBox <gi...@apache.org>.
andymc12 merged pull request #697:
URL: https://github.com/apache/cxf/pull/697
----------------------------------------------------------------
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
[GitHub] [cxf] andymc12 commented on a change in pull request #697: [CXF-8346] TCK Changes for URI and hasEntity checks
Posted by GitBox <gi...@apache.org>.
andymc12 commented on a change in pull request #697:
URL: https://github.com/apache/cxf/pull/697#discussion_r493968775
##########
File path: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/ResponseImpl.java
##########
@@ -137,22 +143,70 @@ public Object getActualEntity() {
return lastEntity != null ? lastEntity : entity;
}
+ @Override
public Object getEntity() {
return InjectionUtils.getEntity(getActualEntity());
}
+ @Override
public boolean hasEntity() {
- return getActualEntity() != null;
+ // per spec, need to check if the stream exists and if it has data.
+ Object actualEntity = getActualEntity();
+ if (actualEntity == null) {
+ return false;
+ } else if (actualEntity instanceof InputStream) {
+ final InputStream is = (InputStream) actualEntity;
+ try {
+ if (is.markSupported()) {
+ is.mark(1);
+ int i = is.read();
+ is.reset();
+ return i != -1;
+ } else {
+ try {
+ if (is.available() > 0) {
+ return true;
+ }
+ } catch (IOException ioe) {
+ //Do nothing
+ }
+ int b = is.read();
+ if (b == -1) {
+ return false;
+ }
+ PushbackInputStream pbis;
+ if (is instanceof PushbackInputStream) {
+ pbis = (PushbackInputStream) is;
+ } else {
+ pbis = new PushbackInputStream(is, 1);
+ if (lastEntity != null) {
Review comment:
I believe that `lastEntity` is used for optimizing the case where the entity is an InputStream and a user calls `readEntity(SomeObject.class)` - lastEntity will then be set to the instance of `SomeObject` so that subsequent readEntity calls can just return that object rather then re-reading it from the stream. But since it is also possible to read an entity as an InputStream, this code covers cases like:
```
response.readEntity(InputStream.class);
if (response.hasEntity()) {
response.readEntity(InputStream.class); // or response.readEntity(SomeObject.class), etc
```
----------------------------------------------------------------
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
[GitHub] [cxf] andymc12 commented on pull request #697: [CXF-8346] TCK Changes for URI and hasEntity checks
Posted by GitBox <gi...@apache.org>.
andymc12 commented on pull request #697:
URL: https://github.com/apache/cxf/pull/697#issuecomment-702902483
@coheigea Sorry about that. I have fixes for the test cases proposed in #703. Can you take a look at them to verify that you and/or @reta are ok with the changes? Thanks
----------------------------------------------------------------
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
[GitHub] [cxf] reta commented on a change in pull request #697: [CXF-8346] TCK Changes for URI and hasEntity checks
Posted by GitBox <gi...@apache.org>.
reta commented on a change in pull request #697:
URL: https://github.com/apache/cxf/pull/697#discussion_r493106096
##########
File path: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/ResponseImpl.java
##########
@@ -137,22 +143,70 @@ public Object getActualEntity() {
return lastEntity != null ? lastEntity : entity;
}
+ @Override
public Object getEntity() {
return InjectionUtils.getEntity(getActualEntity());
}
+ @Override
public boolean hasEntity() {
- return getActualEntity() != null;
+ // per spec, need to check if the stream exists and if it has data.
+ Object actualEntity = getActualEntity();
+ if (actualEntity == null) {
+ return false;
+ } else if (actualEntity instanceof InputStream) {
+ final InputStream is = (InputStream) actualEntity;
+ try {
+ if (is.markSupported()) {
+ is.mark(1);
+ int i = is.read();
+ is.reset();
+ return i != -1;
+ } else {
+ try {
+ if (is.available() > 0) {
+ return true;
+ }
+ } catch (IOException ioe) {
+ //Do nothing
+ }
+ int b = is.read();
Review comment:
This would modify the state of the stream, correct?
----------------------------------------------------------------
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
[GitHub] [cxf] reta commented on a change in pull request #697: [CXF-8346] TCK Changes for URI and hasEntity checks
Posted by GitBox <gi...@apache.org>.
reta commented on a change in pull request #697:
URL: https://github.com/apache/cxf/pull/697#discussion_r493113888
##########
File path: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/ResponseImpl.java
##########
@@ -527,10 +621,47 @@ public String getReasonPhrase() {
return statusEnum != null ? statusEnum.getReasonPhrase() : "";
}
+ @Override
public int getStatusCode() {
return statusCode;
}
};
}
-}
+
+ private enum PrimitiveTypes {
+ BYTE(Byte.class, byte.class) { },
+ SHORT(Short.class, short.class) { },
+ INTEGER(Integer.class, int.class) { },
+ LONG(Long.class, long.class) { },
+ FLOAT(Float.class, float.class) { },
+ DOUBLE(Double.class, double.class) { },
+ BOOLEAN(Boolean.class, boolean.class) { },
+ CHAR(Character.class, char.class) { };
+
+ private final Class<?> wrapper;
+ private final Class<?> primitive;
+
+ PrimitiveTypes(Class<?> wrapper, Class<?> primitive) {
+ this.wrapper = wrapper;
+ this.primitive = primitive;
+ }
+
+ public static PrimitiveTypes forType(Class<?> type) {
+ for (PrimitiveTypes primitive : PrimitiveTypes.values()) {
+ if (primitive.supports(type)) {
+ return primitive;
+ }
+ }
+ return null;
+ }
+
+ public boolean supports(Class<?> type) {
+ return type == wrapper || type == primitive;
+ }
+ }
+
+ private static boolean isBasicType(Class<?> type) {
+ return PrimitiveTypes.forType(type) != null || Number.class.isAssignableFrom(type);
Review comment:
You probably could reduce it to `return type.isPrimitive() || Number.class.isAssignableFrom(type) ||
Boolean.class.isAssignableFrom(type) || Char.class.isAssignableFrom(type));` (PrimitiveTypes is not really needed in this case).
----------------------------------------------------------------
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
[GitHub] [cxf] reta commented on a change in pull request #697: [CXF-8346] TCK Changes for URI and hasEntity checks
Posted by GitBox <gi...@apache.org>.
reta commented on a change in pull request #697:
URL: https://github.com/apache/cxf/pull/697#discussion_r494622436
##########
File path: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/ResponseImpl.java
##########
@@ -137,22 +143,70 @@ public Object getActualEntity() {
return lastEntity != null ? lastEntity : entity;
}
+ @Override
public Object getEntity() {
return InjectionUtils.getEntity(getActualEntity());
}
+ @Override
public boolean hasEntity() {
- return getActualEntity() != null;
+ // per spec, need to check if the stream exists and if it has data.
+ Object actualEntity = getActualEntity();
+ if (actualEntity == null) {
+ return false;
+ } else if (actualEntity instanceof InputStream) {
+ final InputStream is = (InputStream) actualEntity;
+ try {
+ if (is.markSupported()) {
+ is.mark(1);
+ int i = is.read();
+ is.reset();
+ return i != -1;
+ } else {
+ try {
+ if (is.available() > 0) {
+ return true;
+ }
+ } catch (IOException ioe) {
+ //Do nothing
+ }
+ int b = is.read();
+ if (b == -1) {
+ return false;
+ }
+ PushbackInputStream pbis;
+ if (is instanceof PushbackInputStream) {
+ pbis = (PushbackInputStream) is;
+ } else {
+ pbis = new PushbackInputStream(is, 1);
+ if (lastEntity != null) {
Review comment:
👍 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
[GitHub] [cxf] andymc12 merged pull request #697: [CXF-8346] TCK Changes for URI and hasEntity checks
Posted by GitBox <gi...@apache.org>.
andymc12 merged pull request #697:
URL: https://github.com/apache/cxf/pull/697
----------------------------------------------------------------
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
[GitHub] [cxf] reta commented on a change in pull request #697: [CXF-8346] TCK Changes for URI and hasEntity checks
Posted by GitBox <gi...@apache.org>.
reta commented on a change in pull request #697:
URL: https://github.com/apache/cxf/pull/697#discussion_r494622436
##########
File path: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/ResponseImpl.java
##########
@@ -137,22 +143,70 @@ public Object getActualEntity() {
return lastEntity != null ? lastEntity : entity;
}
+ @Override
public Object getEntity() {
return InjectionUtils.getEntity(getActualEntity());
}
+ @Override
public boolean hasEntity() {
- return getActualEntity() != null;
+ // per spec, need to check if the stream exists and if it has data.
+ Object actualEntity = getActualEntity();
+ if (actualEntity == null) {
+ return false;
+ } else if (actualEntity instanceof InputStream) {
+ final InputStream is = (InputStream) actualEntity;
+ try {
+ if (is.markSupported()) {
+ is.mark(1);
+ int i = is.read();
+ is.reset();
+ return i != -1;
+ } else {
+ try {
+ if (is.available() > 0) {
+ return true;
+ }
+ } catch (IOException ioe) {
+ //Do nothing
+ }
+ int b = is.read();
+ if (b == -1) {
+ return false;
+ }
+ PushbackInputStream pbis;
+ if (is instanceof PushbackInputStream) {
+ pbis = (PushbackInputStream) is;
+ } else {
+ pbis = new PushbackInputStream(is, 1);
+ if (lastEntity != null) {
Review comment:
👍 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
[GitHub] [cxf] reta commented on a change in pull request #697: [CXF-8346] TCK Changes for URI and hasEntity checks
Posted by GitBox <gi...@apache.org>.
reta commented on a change in pull request #697:
URL: https://github.com/apache/cxf/pull/697#discussion_r493109081
##########
File path: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/ResponseImpl.java
##########
@@ -218,42 +274,46 @@ private Date doGetDate(String dateHeader) {
: HttpUtils.getHttpDate(value.toString());
}
+ @Override
public EntityTag getEntityTag() {
Object header = metadata.getFirst(HttpHeaders.ETAG);
return header == null || header instanceof EntityTag ? (EntityTag)header
: EntityTag.valueOf(header.toString());
}
+ @Override
public Locale getLanguage() {
Object header = metadata.getFirst(HttpHeaders.CONTENT_LANGUAGE);
return header == null || header instanceof Locale ? (Locale)header
: HttpUtils.getLocale(header.toString());
}
+ @Override
public Date getLastModified() {
return doGetDate(HttpHeaders.LAST_MODIFIED);
}
+ @Override
public int getLength() {
Object header = metadata.getFirst(HttpHeaders.CONTENT_LENGTH);
return HttpUtils.getContentLength(header == null ? null : header.toString());
}
+ @Override
public URI getLocation() {
Object header = metadata.getFirst(HttpHeaders.LOCATION);
- if (header == null && outMessage != null) {
- header = outMessage.get(Message.REQUEST_URI);
- }
- return header == null || header instanceof URI ? (URI) header
+ return header == null || header instanceof URI ? (URI)header
Review comment:
Sorry, why the `outMessage.get(Message.REQUEST_URI)` got removed?
----------------------------------------------------------------
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
[GitHub] [cxf] reta commented on a change in pull request #697: [CXF-8346] TCK Changes for URI and hasEntity checks
Posted by GitBox <gi...@apache.org>.
reta commented on a change in pull request #697:
URL: https://github.com/apache/cxf/pull/697#discussion_r493106096
##########
File path: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/ResponseImpl.java
##########
@@ -137,22 +143,70 @@ public Object getActualEntity() {
return lastEntity != null ? lastEntity : entity;
}
+ @Override
public Object getEntity() {
return InjectionUtils.getEntity(getActualEntity());
}
+ @Override
public boolean hasEntity() {
- return getActualEntity() != null;
+ // per spec, need to check if the stream exists and if it has data.
+ Object actualEntity = getActualEntity();
+ if (actualEntity == null) {
+ return false;
+ } else if (actualEntity instanceof InputStream) {
+ final InputStream is = (InputStream) actualEntity;
+ try {
+ if (is.markSupported()) {
+ is.mark(1);
+ int i = is.read();
+ is.reset();
+ return i != -1;
+ } else {
+ try {
+ if (is.available() > 0) {
+ return true;
+ }
+ } catch (IOException ioe) {
+ //Do nothing
+ }
+ int b = is.read();
Review comment:
This would modify the state of the stream, correct?
##########
File path: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/ResponseImpl.java
##########
@@ -137,22 +143,70 @@ public Object getActualEntity() {
return lastEntity != null ? lastEntity : entity;
}
+ @Override
public Object getEntity() {
return InjectionUtils.getEntity(getActualEntity());
}
+ @Override
public boolean hasEntity() {
- return getActualEntity() != null;
+ // per spec, need to check if the stream exists and if it has data.
+ Object actualEntity = getActualEntity();
+ if (actualEntity == null) {
+ return false;
+ } else if (actualEntity instanceof InputStream) {
+ final InputStream is = (InputStream) actualEntity;
+ try {
+ if (is.markSupported()) {
+ is.mark(1);
+ int i = is.read();
+ is.reset();
+ return i != -1;
+ } else {
+ try {
+ if (is.available() > 0) {
+ return true;
+ }
+ } catch (IOException ioe) {
+ //Do nothing
+ }
+ int b = is.read();
Review comment:
This would modify the state of the stream, correct? May be it would be simpler to check/construct the PushbackInputStream and than do read / unread?
##########
File path: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/ResponseImpl.java
##########
@@ -137,22 +143,70 @@ public Object getActualEntity() {
return lastEntity != null ? lastEntity : entity;
}
+ @Override
public Object getEntity() {
return InjectionUtils.getEntity(getActualEntity());
}
+ @Override
public boolean hasEntity() {
- return getActualEntity() != null;
+ // per spec, need to check if the stream exists and if it has data.
+ Object actualEntity = getActualEntity();
+ if (actualEntity == null) {
+ return false;
+ } else if (actualEntity instanceof InputStream) {
+ final InputStream is = (InputStream) actualEntity;
+ try {
+ if (is.markSupported()) {
+ is.mark(1);
+ int i = is.read();
+ is.reset();
+ return i != -1;
+ } else {
+ try {
+ if (is.available() > 0) {
+ return true;
+ }
+ } catch (IOException ioe) {
+ //Do nothing
+ }
+ int b = is.read();
+ if (b == -1) {
+ return false;
+ }
+ PushbackInputStream pbis;
+ if (is instanceof PushbackInputStream) {
+ pbis = (PushbackInputStream) is;
+ } else {
+ pbis = new PushbackInputStream(is, 1);
+ if (lastEntity != null) {
Review comment:
Unfamiliar with this part, do you have insights about `lastEntity` vs `entity`?
##########
File path: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/ResponseImpl.java
##########
@@ -218,42 +274,46 @@ private Date doGetDate(String dateHeader) {
: HttpUtils.getHttpDate(value.toString());
}
+ @Override
public EntityTag getEntityTag() {
Object header = metadata.getFirst(HttpHeaders.ETAG);
return header == null || header instanceof EntityTag ? (EntityTag)header
: EntityTag.valueOf(header.toString());
}
+ @Override
public Locale getLanguage() {
Object header = metadata.getFirst(HttpHeaders.CONTENT_LANGUAGE);
return header == null || header instanceof Locale ? (Locale)header
: HttpUtils.getLocale(header.toString());
}
+ @Override
public Date getLastModified() {
return doGetDate(HttpHeaders.LAST_MODIFIED);
}
+ @Override
public int getLength() {
Object header = metadata.getFirst(HttpHeaders.CONTENT_LENGTH);
return HttpUtils.getContentLength(header == null ? null : header.toString());
}
+ @Override
public URI getLocation() {
Object header = metadata.getFirst(HttpHeaders.LOCATION);
- if (header == null && outMessage != null) {
- header = outMessage.get(Message.REQUEST_URI);
- }
- return header == null || header instanceof URI ? (URI) header
+ return header == null || header instanceof URI ? (URI)header
Review comment:
Sorry, why the `outMessage.get(Message.REQUEST_URI)` got removed?
##########
File path: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/ResponseImpl.java
##########
@@ -400,12 +472,30 @@ private Link makeAbsoluteLink(Link link) {
responseMessage.put(Message.PROTOCOL_HEADERS, getHeaders());
lastEntity = JAXRSUtils.readFromMessageBodyReader(readers, cls, t,
- anns,
- entityStream,
- mediaType,
- responseMessage);
- autoClose(cls, false);
- return castLastEntity();
+ anns,
+ entityStream,
+ mediaType,
+ responseMessage);
+ // close the entity after readEntity is called.
+ T tCastLastEntity = castLastEntity();
+ shouldClose = shouldClose && !(tCastLastEntity instanceof Closeable)
Review comment:
Shouldn't `AutoCloseable` also be checked?
##########
File path: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/ResponseImpl.java
##########
@@ -527,10 +621,47 @@ public String getReasonPhrase() {
return statusEnum != null ? statusEnum.getReasonPhrase() : "";
}
+ @Override
public int getStatusCode() {
return statusCode;
}
};
}
-}
+
+ private enum PrimitiveTypes {
+ BYTE(Byte.class, byte.class) { },
+ SHORT(Short.class, short.class) { },
+ INTEGER(Integer.class, int.class) { },
+ LONG(Long.class, long.class) { },
+ FLOAT(Float.class, float.class) { },
+ DOUBLE(Double.class, double.class) { },
+ BOOLEAN(Boolean.class, boolean.class) { },
+ CHAR(Character.class, char.class) { };
+
+ private final Class<?> wrapper;
+ private final Class<?> primitive;
+
+ PrimitiveTypes(Class<?> wrapper, Class<?> primitive) {
+ this.wrapper = wrapper;
+ this.primitive = primitive;
+ }
+
+ public static PrimitiveTypes forType(Class<?> type) {
+ for (PrimitiveTypes primitive : PrimitiveTypes.values()) {
+ if (primitive.supports(type)) {
+ return primitive;
+ }
+ }
+ return null;
+ }
+
+ public boolean supports(Class<?> type) {
+ return type == wrapper || type == primitive;
+ }
+ }
+
+ private static boolean isBasicType(Class<?> type) {
+ return PrimitiveTypes.forType(type) != null || Number.class.isAssignableFrom(type);
Review comment:
You probably could reduce it to `return type.isPrimitive() || Number.class.isAssignableFrom(type) ||
Boolean.class.isAssignableFrom(type) || Char.class.isAssignableFrom(type));` (PrimitiveTypes is not really needed in this case).
----------------------------------------------------------------
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