Skip to content

Conversation

@madgnome
Copy link

This commit introduces support for top level Json value
String, true, false, null, number) to Jackson2JsonTokenizer.

Before this change Jackson2JsonTokenizer would only consider token in an object or array and this scenario will fail:

@RestController
public class JsonValueResource {
    @GetMapping("/fail")
    public Mono<Boolean> fail() {
        return WebClient.builder().baseUrl("http://localhost:8000")
                .build().get().uri("/")
                .accept(MediaType.APPLICATION_JSON)
                .retrieve()
                .bodyToMono(Boolean.class)
                .switchIfEmpty(Mono.error(new RuntimeException("No response")));
    }

    @GetMapping
    Mono<Boolean> jsonValueBoolean() {
        return Mono.just(true);
    }

}

Issue: SPR-16166

This commit introduces support for top level Json value
(String, true|false, null, number) to Jackson2JsonTokenizer.

Issue: SPR-16166
@madgnome
Copy link
Author

Codacy/PMD complains about missing asserts in the new tests but I followed the existing pattern in the test. is there a way to ignore?

@bclozel bclozel changed the title Handle top level json value in Jackson2JsonTokenizer SPR-16166: Handle top level json value in Jackson2JsonTokenizer Dec 3, 2017
@poutsma
Copy link
Contributor

poutsma commented Dec 14, 2017

Thank you for providing a PR for this issue! However, I decided to fix it in an alternative, arguably cleaner way. See 8e253a3

@poutsma poutsma closed this Dec 14, 2017
@madgnome
Copy link
Author

Thanks a lot for fixing it @poutsma, your solution is cleaner indeed!

Could you double check that this use case is working?

testTokenizeAsJsonToken(
				singletonList("true"),
 				singletonList(JsonToken.VALUE_TRUE));

@poutsma
Copy link
Contributor

poutsma commented Dec 15, 2017

Unfortunately, the JSON assert framework we use does not support null, true, or false as top-level elements (see this issue). I did test those values in a test project, though, and they worked for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants