Skip to content

[CXF-8346] TCK Changes for URI and hasEntity checks #697

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
andymc12 merged 2 commits into from
Sep 24, 2020
Merged

[CXF-8346] TCK Changes for URI and hasEntity checks #697

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
13459f1
TCK Changes for URI and hasEntity checks
andymc12 Sep 18, 2020
7f9a223
Code review comments
andymc12 Sep 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/ContainerRequestContextImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import javax.ws.rs.core.UriInfo;

import org.apache.cxf.helpers.IOUtils;
import org.apache.cxf.io.DelegatingInputStream;
import org.apache.cxf.jaxrs.utils.ExceptionUtils;
import org.apache.cxf.jaxrs.utils.HttpUtils;
import org.apache.cxf.message.Message;
Expand Down Expand Up @@ -68,12 +69,21 @@ public UriInfo getUriInfo() {

@Override
public boolean hasEntity() {
InputStream is = getEntityStream();
if (is == null) {
return false;
}
// Is Content-Length is explicitly set to 0 ?
if (HttpUtils.isPayloadEmpty(getHeaders())) {
return false;
}

if (is instanceof DelegatingInputStream) {
((DelegatingInputStream) is).cacheInput();
}

try {
return !IOUtils.isEmpty(getEntityStream());
return !IOUtils.isEmpty(is);
} catch (IOException ex) {
throw ExceptionUtils.toInternalServerErrorException(ex, null);
}
Expand All @@ -85,6 +95,7 @@ public void setEntityStream(InputStream is) {
m.setContent(InputStream.class, is);
}

@Override
public MultivaluedMap<String, String> getHeaders() {
h = null;
return HttpUtils.getModifiableStringHeaders(m);
Expand Down Expand Up @@ -112,7 +123,8 @@ public void setRequestUri(URI requestUri) throws IllegalStateException {

protected void doSetRequestUri(URI requestUri) throws IllegalStateException {
checkNotPreMatch();
HttpUtils.resetRequestURI(m, requestUri.getRawPath());
// The JAX-RS TCK requires the full uri toString() rather than just the raw path:
HttpUtils.resetRequestURI(m, requestUri.toString());
String query = requestUri.getRawQuery();
if (query != null) {
m.put(Message.QUERY_STRING, query);
Expand Down
146 changes: 122 additions & 24 deletions rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/ResponseImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.io.ByteArrayInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.PushbackInputStream;
import java.io.Reader;
import java.lang.annotation.Annotation;
import java.lang.reflect.Type;
Expand All @@ -48,11 +49,13 @@
import javax.ws.rs.core.MediaType;
import javax.ws.rs.core.MultivaluedMap;
import javax.ws.rs.core.NewCookie;
import javax.ws.rs.core.NoContentException;
import javax.ws.rs.core.Response;
import javax.ws.rs.core.Response.Status.Family;
import javax.ws.rs.ext.ReaderInterceptor;
import javax.ws.rs.ext.RuntimeDelegate.HeaderDelegate;
import javax.xml.stream.XMLStreamReader;
import javax.xml.transform.Source;

import org.apache.cxf.helpers.IOUtils;
import org.apache.cxf.io.ReaderInputStream;
Expand Down Expand Up @@ -124,10 +127,12 @@ public Message getOutMessage() {
return this.outMessage;
}

@Override
public int getStatus() {
return status.getStatusCode();
}

@Override
public StatusType getStatusInfo() {
return status;
}
Expand All @@ -137,22 +142,69 @@ 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();
Copy link
Member

@reta reta Sep 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@andymc12 andymc12 Sep 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.).

Copy link
Member

@reta reta Sep 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No objections, the reasoning was to keep PushbackInputStream manipulations close by but indeed, you have a good argument in not doing so, thank you.

if (b == -1) {
return false;
}
PushbackInputStream pbis;
if (is instanceof PushbackInputStream) {
pbis = (PushbackInputStream) is;
} else {
pbis = new PushbackInputStream(is, 1);
if (lastEntity != null) {
Copy link
Member

@reta reta Sep 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfamiliar with this part, do you have insights about lastEntity vs entity?

Copy link
Contributor Author

@andymc12 andymc12 Sep 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@reta reta Sep 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 thank you

lastEntity = pbis;
} else {
entity = pbis;
}
}
pbis.unread(b);
return true;
}
} catch (IOException ex) {
throw new ProcessingException(ex);
}
}
return true;
}

@Override
public MultivaluedMap<String, Object> getMetadata() {
return getHeaders();
}

@Override
public MultivaluedMap<String, Object> getHeaders() {
return metadata;
}

@Override
public MultivaluedMap<String, String> getStringHeaders() {
MetadataMap<String, String> headers = new MetadataMap<>(metadata.size());
for (Map.Entry<String, List<Object>> entry : metadata.entrySet()) {
Expand All @@ -162,6 +214,7 @@ public MultivaluedMap<String, String> getStringHeaders() {
return headers;
}

@Override
public String getHeaderString(String header) {
List<Object> methodValues = metadata.get(header);
return HttpUtils.getHeaderString(toListOfStrings(methodValues));
Expand All @@ -181,6 +234,7 @@ private List<String> toListOfStrings(List<Object> values) {
return stringValues;
}

@Override
public Set<String> getAllowedMethods() {
List<Object> methodValues = metadata.get(HttpHeaders.ALLOW);
if (methodValues == null) {
Expand All @@ -193,8 +247,7 @@ public Set<String> getAllowedMethods() {
return methods;
}



@Override
public Map<String, NewCookie> getCookies() {
List<Object> cookieValues = metadata.get(HttpHeaders.SET_COOKIE);
if (cookieValues == null) {
Expand All @@ -208,6 +261,7 @@ public Map<String, NewCookie> getCookies() {
return cookies;
}

@Override
public Date getDate() {
return doGetDate(HttpHeaders.DATE);
}
Expand All @@ -218,42 +272,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
Copy link
Member

@reta reta Sep 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, why the outMessage.get(Message.REQUEST_URI) got removed?

Copy link
Contributor Author

@andymc12 andymc12 Sep 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

: URI.create(header.toString());
}

@Override
public MediaType getMediaType() {
Object header = metadata.getFirst(HttpHeaders.CONTENT_TYPE);
return header == null || header instanceof MediaType ? (MediaType)header
: (MediaType)JAXRSUtils.toMediaType(header.toString());
}

@Override
public boolean hasLink(String relation) {
List<Object> linkValues = metadata.get(HttpHeaders.LINK);
if (linkValues != null) {
Expand All @@ -274,6 +332,7 @@ public boolean hasLink(String relation) {
return false;
}

@Override
public Link getLink(String relation) {
Set<Link> links = getAllLinks();
for (Link link : links) {
Expand All @@ -284,11 +343,13 @@ public Link getLink(String relation) {
return null;
}

@Override
public Link.Builder getLinkBuilder(String relation) {
Link link = getLink(relation);
return link == null ? null : Link.fromLink(link);
}

@Override
public Set<Link> getLinks() {
return new HashSet<>(getAllLinks());
}
Expand Down Expand Up @@ -332,31 +393,40 @@ private List<Link> parseLink(Object o) {
return Collections.unmodifiableList(links);
}

@Override
public <T> T readEntity(Class<T> cls) throws ProcessingException, IllegalStateException {
return readEntity(cls, new Annotation[]{});
}

@Override
public <T> T readEntity(GenericType<T> genType) throws ProcessingException, IllegalStateException {
return readEntity(genType, new Annotation[]{});
}

public <T> T readEntity(Class<T> cls, Annotation[] anns) throws ProcessingException,
IllegalStateException {

return doReadEntity(cls, cls, anns);
@Override
public <T> T readEntity(Class<T> cls, Annotation[] anns) throws ProcessingException, IllegalStateException {
return doReadEntity(cls, cls, anns, true);
}

@Override
@SuppressWarnings("unchecked")
public <T> T readEntity(GenericType<T> genType, Annotation[] anns) throws ProcessingException,
IllegalStateException {
return doReadEntity((Class<T>)genType.getRawType(),
genType.getType(), anns);
public <T> T readEntity(GenericType<T> genType, Annotation[] anns)
throws ProcessingException, IllegalStateException {
return doReadEntity((Class<T>) genType.getRawType(),
genType.getType(), anns, true);
}

public <T> T doReadEntity(Class<T> cls, Type t, Annotation[] anns)
throws ProcessingException, IllegalStateException {
return doReadEntity(cls, t, anns, false);
}

public <T> T doReadEntity(Class<T> cls, Type t, Annotation[] anns) throws ProcessingException,
IllegalStateException {
public <T> T doReadEntity(Class<T> cls, Type t, Annotation[] anns, boolean closeAfterRead)
throws ProcessingException, IllegalStateException {

checkEntityIsClosed();
//according to javadoc, should close when is not buffered.
boolean shouldClose = !this.entityBufferred;

if (lastEntity != null && cls.isAssignableFrom(lastEntity.getClass())
&& !(lastEntity instanceof InputStream)) {
Expand Down Expand Up @@ -400,12 +470,30 @@ public <T> T doReadEntity(Class<T> cls, Type t, Annotation[] anns) throws Proces
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 AutoCloseable)
&& !(tCastLastEntity instanceof Source);
if (closeAfterRead && shouldClose) {
close();
}
return tCastLastEntity;
} catch (NoContentException ex) {
//when content is empty, return null instead of throw exception to pass TCK
//check if basic type. Basic type throw exception, else return null.
if (isBasicType(cls)) {
autoClose(cls, true);
reportMessageHandlerProblem("MSG_READER_PROBLEM", cls, mediaType, ex);
} else {
if (closeAfterRead && shouldClose) {
close();
}
return null;
}
} catch (Exception ex) {
autoClose(cls, true);
reportMessageHandlerProblem("MSG_READER_PROBLEM", cls, mediaType, ex);
Expand Down Expand Up @@ -476,6 +564,7 @@ protected void autoClose(Class<?> cls, boolean exception) {
}
}

@Override
public boolean bufferEntity() throws ProcessingException {
checkEntityIsClosed();
if (!entityBufferred && entity instanceof InputStream) {
Expand All @@ -491,6 +580,7 @@ public boolean bufferEntity() throws ProcessingException {
return entityBufferred;
}

@Override
public void close() throws ProcessingException {
if (!entityClosed) {
if (!entityBufferred && entity instanceof InputStream) {
Expand All @@ -515,10 +605,12 @@ private void checkEntityIsClosed() {
private Response.StatusType createStatusType(int statusCode, String reasonPhrase) {
return new Response.StatusType() {

@Override
public Family getFamily() {
return Response.Status.Family.familyOf(statusCode);
}

@Override
public String getReasonPhrase() {
if (reasonPhrase != null) {
return reasonPhrase;
Expand All @@ -527,10 +619,16 @@ public String getReasonPhrase() {
return statusEnum != null ? statusEnum.getReasonPhrase() : "";
}

@Override
public int getStatusCode() {
return statusCode;
}

};
}
}

private static boolean isBasicType(Class<?> type) {
return type.isPrimitive() || Number.class.isAssignableFrom(type) || Boolean.class.isAssignableFrom(type)
|| Character.class.isAssignableFrom(type);
}
}
Loading