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

Record objects failing silently with Jakarta EE 9.1 #19698

Closed
rdean opened this issue Dec 31, 2021 · 14 comments · Fixed by #26176
Closed

Record objects failing silently with Jakarta EE 9.1 #19698

rdean opened this issue Dec 31, 2021 · 14 comments · Fixed by #26176
Assignees
Projects

Comments

@rdean
Copy link

rdean commented Dec 31, 2021

I'm experimenting with Java 17 and JakartaEE 9.1 restful services and running into issues with Record objects.

I generated a project with a simple greeting service that returns a Record type:

    @GET
    @Path("{name}")
    @Produces(MediaType.APPLICATION_JSON)
    public GreetingResponse greeting(@PathParam("name") String name) throws WebApplicationException {
      return new GreetingResponse("Hello, " + name); 
    }

where GreetingResponse is

public record GreetingResponse(String greeting) { }

When accessing the resource

curl http://localhost:9080/hello-world/api/v1/greeting/Robert

the response is

{}

No error message is produced in the logs.

Yasson was recently updated to support Record types. I flirted with the jsonbContainer-2.0 feature but the documentation is unclear on how to configure in server.xml or where to put the jar / whether or not a <bell> is required.

@WhiteCat22
Copy link
Member

Hi @rdean, thank's for bringing this to our attention. I am out of the office today, but I'll investigate on Monday.

@WhiteCat22
Copy link
Member

Hi @rdean

I think I have this reproduced, or close. What is the HTTP status code of your curl command? When I try to hit my endpoint I'm getting a 404.

My hunch is that our JAX-RS implementation (Resteasy 4.7.2.Final) doesn't yet support record types. At first I thought that the issue was probably that we don't ship a MessageBodyWriter that supports record types. But, with no errors in the logs and getting a 404, I think the error is even earlier than message serialization. Rather, Resteasy isn't finding the endpoint at all.

Here is the basic application I'm using to reproduce with Java 17 temurin

@Path("/")
public class PrototypeResource {

    @GET
    @Path("echo")
    public GreetingResponse hello() {
        return new GreetingResponse("Hello World!");
    }

    public record GreetingResponse(String greeting) { }

}
@ApplicationPath("/")
public class PrototypeApplication extends Application {}

I'm deploying the application via the dropins directory and using the restfulWS-3.0 feature in my server.xml

@rdean
Copy link
Author

rdean commented Jan 3, 2022

I got a 200 OK response with an empty JSON block for payload.

projects/liberty/jee-example is 📦 0.0.1-SNAPSHOT via ☕ v17.0.1 
❯ curl http://localhost:9080/jee9-example/api/v1/salutation/Robert -v
*   Trying 127.0.0.1:9080...
* Connected to localhost (127.0.0.1) port 9080 (#0)
> GET /jee9-example/api/v1/salutation/Robert HTTP/1.1
> Host: localhost:9080
> User-Agent: curl/7.79.1
> Accept: */*
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< Content-Type: application/json
< Content-Language: en-US
< Content-Length: 2
< Date: Mon, 03 Jan 2022 22:51:57 GMT
< 
* Connection #0 to host localhost left intact
{}%                                                                                                                                           

@rdean
Copy link
Author

rdean commented Jan 3, 2022

I added my example to a personal repo on GitHub: https://github.com/rdean/jee-example

@WhiteCat22
Copy link
Member

Thanks!

@WhiteCat22
Copy link
Member

@rdean I was able to reproduce the issue with your provided sample, thank you!

When I enabled Liberty trace I found what I expected:

ss.resteasy.core.providerfactory.ResteasyProviderFactoryImpl 1 getMessageBodyWriter MessageBodyWriter: io.openliberty.restfulWS30.jsonb20provider.JsonBProvider

What this means is that Liberty is selecting our built in JsonBProvider (MessageBodyWriter) to serialize the response object.

I did some searching and found this blog post:
https://rmannibucau.metawerx.net/java-14-record-class-type-and-json-b.html

Basically, JSONB doesn't currently support record types. But that article is from 2019 and references Johnzon, we use Yasson as our JSONB implementation in Open Liberty.

So I found this article:
https://dev.to/cchacin/java-14-records-with-jakartaee-json-b-160n

It's a little bit newer, but references Yasson. Ultimately it says the same thing. Records will likely be supported in future versions of JSONB.

Where does that leave us now? Currently, to serialize record types you'll have to provide your own MessageBodyWriter that supports serialization of java record types to json.

It looks like Jackson 2.12.3 can do this:
https://dev.to/brunooliveira/practical-java-16-using-jackson-to-serialize-records-4og4

@WhiteCat22
Copy link
Member

Here's a blog on how to bring your own Jackson with your application: https://openliberty.io/blog/2020/11/11/byo-jackson.html

@WhiteCat22 WhiteCat22 added this to EE9 in JAX-RS Team Sep 29, 2022
@inad9300
Copy link

Now that Java 17 support is officially claimed, I think it is important to get this fixed (or at least to more visibly communicate about this exception).

@WhiteCat22
Copy link
Member

I took a look and we currently depend on Yasson 2.0.1 for EE9 and 3.0.0. for EE10.

Record support was added to Yasson 2.0.3.

So, this works with EE10, but we need to update EE9 to 2.0.3.

njr-11 added a commit to njr-11/open-liberty that referenced this issue Oct 31, 2022
njr-11 added a commit to njr-11/open-liberty that referenced this issue Oct 31, 2022
njr-11 added a commit to njr-11/open-liberty that referenced this issue Oct 31, 2022
njr-11 added a commit to njr-11/open-liberty that referenced this issue Nov 1, 2022
@bmarwell
Copy link
Contributor

bmarwell commented Dec 7, 2022

Hi @WhiteCat22!
Actually, 2.0.3 is not enough!

2.0.3 does not support :

    final JsonbConfig jsonbConfig =
        new JsonbConfig()
            .withPropertyVisibilityStrategy(new PrivateVisibilityStrategy())
            .withPropertyNamingStrategy(PropertyNamingStrategy.LOWER_CASE_WITH_UNDERSCORES);

When using Jsonb, it will still try to look for a property "firstName" instead of "first_name".
Updating to Yasson 3.x.x fixes this behaviour.

@jhanders34
Copy link
Member

We upgraded to 2.0.4. Does that resolve the issue?

@bmarwell
Copy link
Contributor

bmarwell commented Dec 7, 2022

Nope, 2.0.4 still has the wrong behaviour. Just updated my new IT:
bmarwell/jaxrs-test-showcase#61

Jsonb is instantiated here: https://github.com/bmarwell/jaxrs-test-showcase/blob/70b783a1853c7c35fcf21ac56ebcd64d505cc174/app/web/rest-impl/src/main/java/io/github/jaxrstesting/web/rest/impl/data/DataInitializer.java

Works fine in the unit test:
https://github.com/bmarwell/jaxrs-test-showcase/blob/70b783a1853c7c35fcf21ac56ebcd64d505cc174/app/web/rest-impl/src/test/java/io/github/jaxrstesting/web/rest/impl/data/DataInitializerTest.java

Fails in the Open Liberty IT.

Fails in my unit test when I downgrade from 3.0.2 to 2.0.4.

To execute the test, use mvn verify. You can change the version in the pom.xml of the PR to see for yourself. Always reproducible.

Workaround: use the jsonbContainer + bells-Feature (untested):

<server>
  <featureManager>
    <feature>jsonbContainer-1.0</feature>
  </featureManager>

  <bell libraryRef="johnzon"/>

  <library id="johnzon">
    <fileset dir="${server.config.dir}/johnzon"includes="*.jar"/>
  </library>
</server>

@bmarwell
Copy link
Contributor

bmarwell commented Dec 8, 2022

I dug into this a little further. Neither yasson 3 can be used (different API, would need jsonbContainer-3.0), nor johnzon (Uses classifiers, which is not supported by Liberty).

However, adding some null checks revealed that this is in fact a problem in Yasson. Yasson seems to ignore the PropertyNamingStrategy in any version of 2.x / 3.x. Will open a bug over there.

// edit: eclipse-ee4j/yasson#583

@bmarwell
Copy link
Contributor

... here's the PR
eclipse-ee4j/yasson#584

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

Successfully merging a pull request may close this issue.

7 participants