Skip to content
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.

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

Provide a way to automatically deserialize non-OK JSON response #5002

Merged
merged 30 commits into from
Apr 11, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
476442a
add the poc code of @jrhee17
my4-dev Jun 10, 2023
30bc8aa
Provide a way to automatically deserialize non-OK JSON response
my4-dev Jul 1, 2023
f6c8c80
Merge branch 'main' into issue-4382-new
my4-dev Jul 2, 2023
3182531
move andThenJson methods from BlockingResponseAs to ResponseAs in ord…
my4-dev Jul 5, 2023
a228219
fix to throw InvalidHttpResponseException in case predicate.test is f…
my4-dev Jul 6, 2023
89bd77a
fix to throw InvalidHttpResponseException in case predicate.test is f…
my4-dev Jul 6, 2023
2b9c62c
Merge branch 'main' into issue-4382-new
my4-dev Jul 6, 2023
a800fe9
remove BlockingResponseAs and revert
my4-dev Jul 8, 2023
284d469
apply comments
my4-dev Jul 8, 2023
e2ac43c
delete HttpStatus[Class]Predicate
my4-dev Jul 8, 2023
256acf3
rename andThenJson to json in ResponseAs
my4-dev Jul 10, 2023
8f428cc
revert and format
my4-dev Jul 20, 2023
532526a
apply comments
my4-dev Jul 20, 2023
f6e8c4a
add JavaDoc and header
my4-dev Jul 21, 2023
2b36e1e
Merge branch 'main' into issue-4382-new
my4-dev Jul 22, 2023
7617569
Merge branch 'main' into issue-4382-new
jrhee17 Aug 14, 2023
c6617b8
fix failing test
jrhee17 Aug 14, 2023
9abae16
minor documentation updates
jrhee17 Aug 14, 2023
c2c0f48
lint
my4-dev Aug 16, 2023
89b15a2
Merge branch 'main' into issue-4382-new
my4-dev Oct 14, 2023
8be91f8
apply minor comments
my4-dev Oct 14, 2023
37b809d
rename json methods which return BlockingConditionalResponseAs to blo…
my4-dev Oct 14, 2023
889f5c7
re-defined DefaultConditionalResponseAs as the public class
my4-dev Oct 14, 2023
ff99ed6
add method variants of json
my4-dev Oct 14, 2023
a434b8c
lint
my4-dev Oct 14, 2023
c944bd3
Use orElseJson
Oct 16, 2023
10e7b45
Fix docs
minwoox Oct 17, 2023
a6660c8
More fix
minwoox Oct 17, 2023
6f1cf3a
Merge branch 'main' into issue-4382-new
jrhee17 Apr 4, 2024
146ba11
Merge branch 'main' into issue-4382-new
jrhee17 Apr 11, 2024
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
Prev Previous commit
Next Next commit
Provide a way to automatically deserialize non-OK JSON response
  • Loading branch information
my4-dev committed Jul 1, 2023
commit 30bc8aa6f9395c4cb4cce4a15fe3dbeacabd715a
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,6 @@ static <T> ResponseAs<AggregatedHttpResponse, ResponseEntity<T>> json(TypeRefere

private static <T> ResponseEntity<T> newJsonResponseEntity(AggregatedHttpResponse response,
JsonDecoder<T> decoder) {
if (!response.status().isSuccess()) {
jrhee17 marked this conversation as resolved.
Show resolved Hide resolved
throw newInvalidHttpResponseException(response);
}

try {
return ResponseEntity.of(response.headers(), decoder.decode(response.content().array()),
response.trailers());
Expand All @@ -69,13 +65,6 @@ private static <T> ResponseEntity<T> newJsonResponseEntity(AggregatedHttpRespons
}
}

private static InvalidHttpResponseException newInvalidHttpResponseException(
AggregatedHttpResponse response) {
return new InvalidHttpResponseException(
response, "status: " + response.status() +
" (expect: the success class (2xx). response: " + response, null);
}

@FunctionalInterface
private interface JsonDecoder<T> {
T decode(byte[] bytes) throws IOException;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,19 @@
/*
* Copyright 2023 LINE Corporation
*
* LINE Corporation licenses this file to you under the Apache License,
* version 2.0 (the "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at:
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations
* under the License.
*/

package com.linecorp.armeria.client;

import java.util.function.Predicate;
Expand All @@ -9,47 +25,85 @@
import com.linecorp.armeria.common.HttpResponse;
import com.linecorp.armeria.common.ResponseEntity;

jrhee17 marked this conversation as resolved.
Show resolved Hide resolved
final class BlockingConditionalResponseAs<V> extends ConditionalResponseAs<HttpResponse, AggregatedHttpResponse, ResponseEntity<V>> {
public final class BlockingConditionalResponseAs<V>
extends ConditionalResponseAs<HttpResponse, AggregatedHttpResponse, ResponseEntity<V>> {

BlockingConditionalResponseAs(ResponseAs<HttpResponse, AggregatedHttpResponse> originalResponseAs,
ResponseAs<AggregatedHttpResponse, ResponseEntity<V>> responseAs,
Predicate<AggregatedHttpResponse> predicate) {
super(originalResponseAs, responseAs, predicate);
}

/**
* Invokes {@link ConditionalResponseAs#andThen(ResponseAs, Predicate)} to add the mapping of
* {@link ResponseAs} and {@link Predicate}
*/
public BlockingConditionalResponseAs<V> andThenJson(
Copy link
Member

@minwoox minwoox Oct 13, 2023

Choose a reason for hiding this comment

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

I think we should rename this to orElseJson. andThen is used when both instances are used. However, if predicate returns false, then it isn't converted.
Let me send a patch to rename these methods and address comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks.
Please send me your suggestion for these method's name.

Copy link
Member

@minwoox minwoox Oct 17, 2023

Choose a reason for hiding this comment

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

Hi, @my4-dev! I made a few changes:

  • Do not block the inside of the converter
    • I realized that this chaining converts also can be used by the WebClient. We must not block the inside of the converters.
  • Rename andThenJson to orElseJson
  • Remove ConditionalResponseAs interface because we don't need it at the moment.

PTAL and let me know if there are changes that I need to revert. 😉

Class<? extends V> clazz, Predicate<AggregatedHttpResponse> predicate) {
return (BlockingConditionalResponseAs<V>) andThen(AggregatedResponseAs.json(clazz), predicate);
}

/**
* Invokes {@link ConditionalResponseAs#andThen(ResponseAs, Predicate)} to add the mapping of
* {@link ResponseAs} and {@link Predicate}
*/
public BlockingConditionalResponseAs<V> andThenJson(
Class<? extends V> clazz, ObjectMapper objectMapper, Predicate<AggregatedHttpResponse> predicate) {
return (BlockingConditionalResponseAs<V>) andThen(AggregatedResponseAs.json(clazz, objectMapper), predicate);
return (BlockingConditionalResponseAs<V>) andThen(
AggregatedResponseAs.json(clazz, objectMapper), predicate);
}

/**
* Invokes {@link ConditionalResponseAs#andThen(ResponseAs, Predicate)} to add the mapping of
* {@link ResponseAs} and {@link Predicate}
*/
public BlockingConditionalResponseAs<V> andThenJson(
TypeReference<? extends V> typeRef, Predicate<AggregatedHttpResponse> predicate) {
return (BlockingConditionalResponseAs<V>) andThen(AggregatedResponseAs.json(typeRef), predicate);
}

/**
* Invokes {@link ConditionalResponseAs#andThen(ResponseAs, Predicate)} to add the mapping of
* {@link ResponseAs} and {@link Predicate}
*/
public BlockingConditionalResponseAs<V> andThenJson(
TypeReference<? extends V> typeRef, ObjectMapper objectMapper, Predicate<AggregatedHttpResponse> predicate) {
return (BlockingConditionalResponseAs<V>) andThen(AggregatedResponseAs.json(typeRef, objectMapper), predicate);
TypeReference<? extends V> typeRef, ObjectMapper objectMapper,
Predicate<AggregatedHttpResponse> predicate) {
return (BlockingConditionalResponseAs<V>) andThen(
AggregatedResponseAs.json(typeRef, objectMapper), predicate);
}

/**
* Invokes {@link ConditionalResponseAs#orElse(ResponseAs)} and returns {@link ResponseAs} whose
* {@link Predicate} is evaluated as true.
*/
public ResponseAs<HttpResponse, ResponseEntity<V>> orElseJson(Class<? extends V> clazz) {
return super.orElse(AggregatedResponseAs.json(clazz));
return orElse(AggregatedResponseAs.json(clazz));
}

public ResponseAs<HttpResponse, ResponseEntity<V>> orElseJson(Class<? extends V> clazz, ObjectMapper objectMapper) {
return super.orElse(AggregatedResponseAs.json(clazz, objectMapper));
/**
* Invokes {@link ConditionalResponseAs#orElse(ResponseAs)} and returns {@link ResponseAs} whose
* {@link Predicate} is evaluated as true.
*/
public ResponseAs<HttpResponse, ResponseEntity<V>> orElseJson(
Class<? extends V> clazz, ObjectMapper objectMapper) {
return orElse(AggregatedResponseAs.json(clazz, objectMapper));
}

/**
* Invokes {@link ConditionalResponseAs#orElse(ResponseAs)} and returns {@link ResponseAs} whose
* {@link Predicate} is evaluated as true.
*/
public ResponseAs<HttpResponse, ResponseEntity<V>> orElseJson(TypeReference<? extends V> typeRef) {
return super.orElse(AggregatedResponseAs.json(typeRef));
return orElse(AggregatedResponseAs.json(typeRef));
}

public ResponseAs<HttpResponse, ResponseEntity<V>> orElseJson(TypeReference<? extends V> typeRef, ObjectMapper objectMapper) {
return super.orElse(AggregatedResponseAs.json(typeRef, objectMapper));
/**
* Invokes {@link ConditionalResponseAs#orElse(ResponseAs)} and returns {@link ResponseAs} whose
* {@link Predicate} is evaluated as true.
*/
public ResponseAs<HttpResponse, ResponseEntity<V>> orElseJson(
TypeReference<? extends V> typeRef, ObjectMapper objectMapper) {
return orElse(AggregatedResponseAs.json(typeRef, objectMapper));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@

import java.util.function.Predicate;

import com.fasterxml.jackson.core.type.TypeReference;
import com.fasterxml.jackson.databind.ObjectMapper;

import com.linecorp.armeria.common.AggregatedHttpResponse;
import com.linecorp.armeria.common.HttpResponse;
import com.linecorp.armeria.common.util.Exceptions;
Expand All @@ -41,7 +44,26 @@ public boolean requiresAggregation() {
return true;
}

<V> BlockingConditionalResponseAs<V> andThenJson(Class<? extends V> clazz, Predicate<AggregatedHttpResponse> predicate) {
public <V> BlockingConditionalResponseAs<V> andThenJson(
Class<? extends V> clazz, Predicate<AggregatedHttpResponse> predicate) {
return new BlockingConditionalResponseAs<>(this, AggregatedResponseAs.json(clazz), predicate);
}

public <V> BlockingConditionalResponseAs<V> andThenJson(
Class<? extends V> clazz, ObjectMapper objectMapper, Predicate<AggregatedHttpResponse> predicate) {
return new BlockingConditionalResponseAs<>(
this, AggregatedResponseAs.json(clazz, objectMapper), predicate);
}

public <V> BlockingConditionalResponseAs<V> andThenJson(
TypeReference<? extends V> typeRef, Predicate<AggregatedHttpResponse> predicate) {
return new BlockingConditionalResponseAs<>(this, AggregatedResponseAs.json(typeRef), predicate);
}

public <V> BlockingConditionalResponseAs<V> andThenJson(
TypeReference<? extends V> typeRef, ObjectMapper objectMapper,
Predicate<AggregatedHttpResponse> predicate) {
return new BlockingConditionalResponseAs<>(
this, AggregatedResponseAs.json(typeRef, objectMapper), predicate);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,23 +22,38 @@
import java.util.Map.Entry;
import java.util.function.Predicate;

/**
* Adds the mapping of {@link ResponseAs} and {@link Predicate} in order to return {@link ResponseAs} whose
* {@link Predicate} is evaluated as true.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
*/
@UnstableApi

public class ConditionalResponseAs<T, R, V> {

private final ResponseAs<T, R> originalResposeAs;
ConditionalResponseAs (ResponseAs<T, R> originalResposeAs, ResponseAs<R, V> responseAs, Predicate<R> predicate) {
this.originalResposeAs = originalResposeAs;
// This is necessary to add predicate into list
private final ResponseAs<T, R> originalResponseAs;
private final List<Entry<ResponseAs<R, V>, Predicate<R>>> predicateMappingList = new ArrayList<>();

ConditionalResponseAs(
ResponseAs<T, R> originalResponseAs, ResponseAs<R, V> responseAs, Predicate<R> predicate) {
this.originalResponseAs = originalResponseAs;
andThen(responseAs, predicate);
}

final List<Entry<ResponseAs<R, V>, Predicate<R>>> list = new ArrayList<>();
/**
* Adds the mapping of {@link ResponseAs} and {@link Predicate} to the List.
*/
public ConditionalResponseAs<T, R, V> andThen(ResponseAs<R, V> responseAs, Predicate<R> predicate) {
predicateMappingList.add(new SimpleEntry<>(responseAs, predicate));
return this;
}

/**
* Returns {@link ResponseAs} whose {@link Predicate} is evaluated as true.
*/
public ResponseAs<T, V> orElse(ResponseAs<R, V> responseAs) {
return new ResponseAs<T, V>() {
@Override
public V as(T response) {
final R r = originalResposeAs.as(response);
for (Entry<ResponseAs<R, V>, Predicate<R>> entry: list) {
final R r = originalResponseAs.as(response);
for (Entry<ResponseAs<R, V>, Predicate<R>> entry: predicateMappingList) {
if (entry.getValue().test(r)) {
return entry.getKey().as(r);
}
Expand All @@ -47,9 +62,4 @@ public V as(T response) {
}
};
}

public ConditionalResponseAs<T, R, V> andThen(ResponseAs<R, V> responseAs, Predicate<R> predicate) {
list.add(new SimpleEntry<>(responseAs, predicate));
return this;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
/*
* Copyright 2023 LINE Corporation
*
* LINE Corporation licenses this file to you under the Apache License,
* version 2.0 (the "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at:
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations
* under the License.
*/

package com.linecorp.armeria.client;

import static java.util.Objects.requireNonNull;

import java.util.function.Predicate;

import com.linecorp.armeria.common.HttpStatus;
import com.linecorp.armeria.common.HttpStatusClass;

final class HttpStatusClassPredicates implements Predicate<HttpStatus> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wasn't sure how this predicate plays into the current APIs as nothing is public 😅
I'm also curious how the HttpStatus[Class]Predicates tie into the new APIs since. Can you show me an example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the same too.
I'm not sure how to use HttpStatus[Class]Predicate in the new APIs.
It would be unnecessary so that I would remove this class.


private static final HttpStatusClassPredicates INFORMATIONAL_PREDICATE =
new HttpStatusClassPredicates(HttpStatusClass.INFORMATIONAL);

private static final HttpStatusClassPredicates SUCCESS_PREDICATE =
new HttpStatusClassPredicates(HttpStatusClass.SUCCESS);

private static final HttpStatusClassPredicates REDIRECTION_PREDICATE =
new HttpStatusClassPredicates(HttpStatusClass.REDIRECTION);

private static final HttpStatusClassPredicates CLIENT_ERROR_PREDICATE =
new HttpStatusClassPredicates(HttpStatusClass.CLIENT_ERROR);

private static final HttpStatusClassPredicates SERVER_ERROR_PREDICATE =
new HttpStatusClassPredicates(HttpStatusClass.SERVER_ERROR);

private static final HttpStatusClassPredicates UNKNOWN_PREDICATE =
new HttpStatusClassPredicates(HttpStatusClass.UNKNOWN);

static HttpStatusClassPredicates of(HttpStatusClass httpStatusClass) {
switch (httpStatusClass) {
case INFORMATIONAL:
return INFORMATIONAL_PREDICATE;
case SUCCESS:
return SUCCESS_PREDICATE;
case REDIRECTION:
return REDIRECTION_PREDICATE;
case CLIENT_ERROR:
return CLIENT_ERROR_PREDICATE;
case SERVER_ERROR:
return SERVER_ERROR_PREDICATE;
default:
return UNKNOWN_PREDICATE;
}
}

private final HttpStatusClass httpStatusClass;

private HttpStatusClassPredicates(HttpStatusClass httpStatusClass) {
this.httpStatusClass = requireNonNull(httpStatusClass, "httpStatusClass");
}

@Override
public boolean test(HttpStatus status) {
return httpStatusClass.contains(status);
}

HttpStatusClass statusClass() {
return httpStatusClass;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
/*
* Copyright 2023 LINE Corporation
*
* LINE Corporation licenses this file to you under the Apache License,
* version 2.0 (the "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at:
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations
* under the License.
*/

package com.linecorp.armeria.client;

import static java.util.Objects.requireNonNull;

import java.util.function.Predicate;

import com.google.common.collect.ImmutableMap;

import com.linecorp.armeria.common.HttpStatus;

final class HttpStatusPredicate implements Predicate<HttpStatus> {
private static final ImmutableMap<HttpStatus, HttpStatusPredicate> httpStatusPredicateMap;

static {
final ImmutableMap.Builder<HttpStatus, HttpStatusPredicate> builder =
ImmutableMap.builderWithExpectedSize(1000);
for (int i = 0; i < 1000; i++) {
final HttpStatus status = HttpStatus.valueOf(i);
builder.put(status, new HttpStatusPredicate(HttpStatus.valueOf(i)));
}
httpStatusPredicateMap = builder.build();
}

static HttpStatusPredicate of(HttpStatus httpStatus) {
if (httpStatus.code() < 0 || httpStatus.code() >= 1000) {
return new HttpStatusPredicate(httpStatus);
} else {
final HttpStatusPredicate httpStatusPredicate = httpStatusPredicateMap.get(httpStatus);
return requireNonNull(httpStatusPredicate, "httpStatusPredicate");
}
}

private final HttpStatus status;

private HttpStatusPredicate(HttpStatus status) {
this.status = requireNonNull(status, "status");
}

@Override
public boolean test(HttpStatus status) {
return this.status.equals(status);
}

HttpStatus status() {
return status;
}
}
Loading