Support ArgumentValue wrapper for input parameters by rwo-trackunit · Pull Request #1450 · kobylynskyi/graphql-java-codegen

rwo-trackunit · 2024-01-16T13:05:23Z

Description

When updating an object we want to support patch-like procedure:

If value is not send, do not change the value
If value is null set the value to null
If value is set, set that value

In #966 it was debated if we could use Optional to solve this, ultimately concluding that it would not be appropriate.

Java 8's Optional was mainly intended for return values from methods, and not for properties of Java classes, as described in Optional in Java SE 8

This PR tries to solve it by using the existing ArgumentValue wrapper for input parameters, when that input is not mandatory and is not a list.

Example:

input PersonLocationInput {
    location: CoordinatesInput!
    age: Float
    name: String = null
    description: String = "a very good desc"
}

becomes

public class PersonLocationInput {
    private CoordinatesInput location;
    private ArgumentValue<Double> age = ArgumentValue.omitted();
    private ArgumentValue<String> name = ArgumentValue.ofNullable(null);
    private ArgumentValue<String> description = ArgumentValue.ofNullable("a very good desc");
    
    ...

This works because ArgumentValue (just like Optional) is supported by spring-graphql GraphQlArgumentBinder
It is all locked behind the configuration useWrapperForNullableInputTypes.

TODO:

As I have no experience with Kotlin or Scala, this currently only works for Java.

Changes were made to:

Codegen library - Java
Codegen library - Kotlin
Codegen library - Scala

jxnu-liguobin · 2024-01-16T15:00:36Z

The optional specification of Java SE8 is not applicable to scala and kotlin. In the functional programming, the data class has accurately described the data type. Therefore, I think only Java needs it.

rwo-trackunit · 2024-01-16T15:05:30Z

I could also use some suggestions on naming: undefined smells a bit like Javascript.

jxnu-liguobin · 2024-01-16T15:10:46Z

I could also use some suggestions on naming: undefined smells a bit like Javascript.

Maybe we don't need it, only isDefined is needed.

rwo-trackunit · 2024-01-16T15:18:51Z

I could also use some suggestions on naming: undefined smells a bit like Javascript.

Maybe we don't need it, only isDefined is needed.

Yea, I should only have one of Defined/Undefined. Maybe we should adopt the Optional.isPresent() naming. And .get and .orElse Although I'm keen to show this is not just a homemade Optional.
Why is naming always the hardest part of coding?

jxnu-liguobin · 2024-01-17T01:01:49Z

I could also use some suggestions on naming: undefined smells a bit like Javascript.

Maybe we don't need it, only isDefined is needed.

Yea, I should only have one of Defined/Undefined. Maybe we should adopt the Optional.isPresent() naming. And .get and .orElse Although I'm keen to show this is not just a homemade Optional. Why is naming always the hardest part of coding?

People from different languages and backgrounds have different norms. 😄

jxnu-liguobin · 2024-01-17T11:49:32Z

It looks good, just describe the option in the markdown document and wait for @kobylynskyi review.

…rgumentValue from org.springframework.graphql.data

rwo-trackunit · 2024-01-17T15:44:58Z

After some feedback, this has been rewritten to use ArgumentValue, rather than generating its own class that solves the same problem.

Which also solves the naming problem, by adopting someone elses.

lilianchiassai · 2024-01-22T01:39:23Z

Thanks for the PR. If projects are not based on spring-graphql, it might be valuable to let the wrapper type up to the user, like this:
<useWrapperForNullableInputTypes>org.springframework.graphql.data.ArgumentValue</useWrapperForNullableInputTypes>
<useWrapperForNullableInputTypes>java.util.Optional</useWrapperForNullableInputTypes>
<useWrapperForNullableInputTypes>custom.example.Undefined</useWrapperForNullableInputTypes>

rwo-trackunit · 2024-01-23T21:18:57Z

Thanks for the PR. If projects are not based on spring-graphql, it might be valuable to let the wrapper type up to the user, like this: <useWrapperForNullableInputTypes>org.springframework.graphql.data.ArgumentValue</useWrapperForNullableInputTypes> <useWrapperForNullableInputTypes>java.util.Optional</useWrapperForNullableInputTypes> <useWrapperForNullableInputTypes>custom.example.Undefined</useWrapperForNullableInputTypes>

I thought about that, and even have had it compatible with Optional at an earlier point in the PoC.
But to support default values, each wrapper type requires 3 string replacement strings to support fx. ArgumentValue.omitted() vs Optional.empty(). Adding that in config quickly becomes very messy.

Alternative solution would be an interface, but ArgumentValue doesn't extend one.

Last good idea is a factory, that is default provided supporting ArgumentValue, but could be replaced with a custom factory.
That could work in general, but could never work with spring-graphql, since GraphQlArgumentBinder only supports Optional and ArgumentValue wrappers.

Other alternative solution would be a custom object provided by this project, but that doesn't support spring-graphql for the same reason.

If there is further request for some solution to support more wrappers (be it the factory or some other good idea), I'm happy to code it up. But I feel this PR is big enough.

kobylynskyi

@rwo-trackunit good job! Your changes look good. I will merge this soon, wait for some other PRs to be merged and release it ASAP.
Thanks!

rwo-trackunit · 2024-02-02T08:01:31Z

Once merged, don't hesitate to tag me in any found bugs, or suggested changes to this corner of the project.

rwo-trackunit · 2024-02-02T08:48:45Z

Consider squashing this PR. The commits in here includes several PoCs, so it may give a confusing git history.

kobylynskyi · 2024-02-24T16:58:55Z

This was released in 5.10.0. Thanks!

rwo-trackunit added 12 commits January 11, 2024 13:51

optional POC

7b443cc

optional POC

7aff014

optional POC

6f66d6a

optional POC

dba9bab

new config

d476312

graphql input param wrapper

9d2b5b9

add tests

15ffda6

cleanup for PR

834fd52

fix build error

88e12d7

cleanup code style

0a1b94d

cleanup code style

8bcfce6

cleanup code style

dd8ea35

jxnu-liguobin reviewed Jan 16, 2024

View reviewed changes

Comment thread src/main/java/com/kobylynskyi/graphql/codegen/model/MappingContext.java

rwo-trackunit added 2 commits January 17, 2024 10:04

update SBT codegen. General code clean.

da66455

code style

9ad5b73

rwo-trackunit added 2 commits January 17, 2024 13:51

update documentation

e6933bb

Update feature from generating GraphQLInputParameter, to relying on A…

e112eb8

…rgumentValue from org.springframework.graphql.data

update link in description

4c239fc

Delete .java-version

886bd29

kobylynskyi approved these changes Feb 1, 2024

View reviewed changes

kobylynskyi changed the title ~~Support undefined input parameter~~ Support ArgumentValue wrapper for input parameters Feb 1, 2024

kobylynskyi added this to the 5.10.0 milestone Feb 1, 2024

kobylynskyi merged commit 3ac1021 into kobylynskyi:main Feb 24, 2024

kobylynskyi added the enhancement New feature or request label Feb 24, 2024

rwo-trackunit deleted the graphql-input-parameter branch February 24, 2024 17:33

bczerwinski-equinix mentioned this pull request Oct 30, 2024

Support ArgumentValue wrapper for input parameters in Kotlin generator #1575

Open

Uh oh!

Conversation

rwo-trackunit commented Jan 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Example:

TODO:

Uh oh!

jxnu-liguobin commented Jan 16, 2024

Uh oh!

rwo-trackunit commented Jan 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jxnu-liguobin commented Jan 16, 2024

Uh oh!

rwo-trackunit commented Jan 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jxnu-liguobin commented Jan 17, 2024

Uh oh!

jxnu-liguobin commented Jan 17, 2024

Uh oh!

rwo-trackunit commented Jan 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lilianchiassai commented Jan 22, 2024

Uh oh!

rwo-trackunit commented Jan 23, 2024

Uh oh!

kobylynskyi left a comment

Choose a reason for hiding this comment

Uh oh!

rwo-trackunit commented Feb 2, 2024

Uh oh!

rwo-trackunit commented Feb 2, 2024

Uh oh!

kobylynskyi commented Feb 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rwo-trackunit commented Jan 16, 2024 •

edited

Loading

rwo-trackunit commented Jan 16, 2024 •

edited

Loading

rwo-trackunit commented Jan 16, 2024 •

edited

Loading

rwo-trackunit commented Jan 17, 2024 •

edited

Loading