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

typesafe client: @OutputOnly #524

Closed
wants to merge 2 commits into from
Closed

typesafe client: @OutputOnly #524

wants to merge 2 commits into from

Conversation

t1
Copy link
Collaborator

@t1 t1 commented Nov 20, 2020

fixes #522 and a minor bug when an error has a path to a nested field but the container value is null

t1 added 2 commits November 19, 2020 18:38
Signed-off-by: Rüdiger zu Dohna <[email protected]>
Signed-off-by: Rüdiger zu Dohna <[email protected]>
@t1 t1 added bug Something isn't working Client This issue applies to the Client side enhancement New feature or request labels Nov 20, 2020
@t1 t1 added this to the 1.0.16 milestone Nov 20, 2020
Copy link
Member

@phillip-kruger phillip-kruger left a comment

Choose a reason for hiding this comment

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

As I said in the issue, I do not think we should introduce this new annotation, but rather use @Ignore as on the server.

@t1
Copy link
Collaborator Author

t1 commented Nov 20, 2020

I just non-suggestively asked the developers in my team. All 5 of them said the same thing: using @Ignore on the setter needs a lot of explanation and learning; a separate @OutputOnly is much easier to understand. We probably should think about adding this to the server, too.

@phillip-kruger
Copy link
Member

We have discussed this and decided to align with JsonB.
See https://javaee.github.io/jsonb-spec/users-guide.html#ignoring-properties

@phillip-kruger
Copy link
Member

@t1 - The client should also support the JsonB annotation (as the server do) @JsonbTransient and other JsonB annotations.

@phillip-kruger
Copy link
Member

There are also other annotations that work this way (field = both, setter = input and getter=output).
@Name , DateFormat, NumberFormat , Description and so on. The client should work the same way.

See for example https://download.eclipse.org/microprofile/microprofile-graphql-1.0.3/microprofile-graphql.html#naming

@phillip-kruger phillip-kruger removed this from the 1.0.16 milestone Nov 20, 2020
@t1
Copy link
Collaborator Author

t1 commented Nov 20, 2020

@phillip-kruger : yes, you're right with everything you say. This is just not where the client currently is!

Annotating a setter as @JsonbTransient makes a lot of sense from the perspective of JSON-B where you think about de/serialization. But in the context of a class used as a client side GraphQL Input, it's strange to annotate a setter as @Ignore... there is a valid reasoning behind that, but it doesn't come naturally. A separate annotation would make things much easier to grasp.

@phillip-kruger
Copy link
Member

@andymc12 - how does RESTClient do this ? Since JAX-RS also support JsonB annotation, does RESTClient do the same ?

@t1 - We should rather put effort into getting the client where it needs to be, than add annotations that later potentially will have to be removed.

@t1
Copy link
Collaborator Author

t1 commented Nov 20, 2020

Let's talk about this in the next meeting

@phillip-kruger
Copy link
Member

Would #316 solve this in another way (so you can now in your project define this an an alias ?)

@t1
Copy link
Collaborator Author

t1 commented Nov 25, 2020

Would #316 solve this in another way (so you can now in your project define this an an alias ?)

We would need way to move a @OutputOnly field-level Stereotype annotation to a @Ignore annotation on the setter... possible, but not here, yet.

@phillip-kruger
Copy link
Member

Closing, as we decided to do @ignore(on=INPUT) and this will first need some changes in MicroProfile API

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Client This issue applies to the Client side enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for @Ignore(on=Type) or @Ignore(on=Input)
2 participants