Skip to content

GraphQlArgumentInstantiator support for patchable arguments #228

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

Closed
dchan4 opened this issue Dec 17, 2021 · 11 comments
Closed

GraphQlArgumentInstantiator support for patchable arguments #228

dchan4 opened this issue Dec 17, 2021 · 11 comments
Labels
status: superseded Issue is superseded by another

Comments

@dchan4
Copy link

dchan4 commented Dec 17, 2021

Currently using the @Argument annotation it is not possible to tell if a field was not provided, or provided with a null. For example given 2 inputs

{
"id": "1"
}

and

{
"id": "1",
"name": null
}

the org.springframework.graphql.data.method.annotation.support.GraphQlArgumentInstantiator#instantiate method will instantiate the name property to null in both cases.

The use case for this is to implement patchable mutations e.g. if the name field is not provided, do not update it.

Workaround is to not use @Argument and implement custom deserialisation using the data fetching environment directly.

@bclozel
Copy link
Member

bclozel commented Dec 17, 2021

This is being discussed as well in #140.
While this seems to be a recurring theme with GraphQL, it is a bit unusual in the Java space as there's no built-in language type to drive that information.

Could you tell us more about this? How would you model that at the type level? How would you persist that with your entities/repository?

@bclozel bclozel added the status: waiting-for-feedback We need additional information before we can continue label Dec 17, 2021
@dchan4
Copy link
Author

dchan4 commented Dec 21, 2021

Indeed there doesn't seem to be a built-in type, so we've had to implement our own 'tri state' type, where the state can be 'update', 'delete', or 'ignore'.

  • ignore - field is not provided
  • delete - field is set to null
  • update - field is set to a value

We use a custom deserialiser in Jackson that can deserialise from JSON to models with this type.

class PatchableDeserializer(
    private val valueType: JavaType? = null
) :
    JsonDeserializer<Patchable<*>>(),
    ContextualDeserializer {
    override fun createContextual(ctxt: DeserializationContext, property: BeanProperty): JsonDeserializer<*> {
        val wrapperType = property.type
        val type = wrapperType.containedType(0)

        return PatchableDeserializer(type)
    }

    override fun deserialize(p: JsonParser, ctxt: DeserializationContext): Patchable<*> {
        return Patchable.update<Any>(ctxt.readValue(p, valueType))
    }

    override fun getNullValue(ctxt: DeserializationContext): Patchable<*> =
        if (ctxt.parser.currentToken == JsonToken.VALUE_NULL)
            Patchable.delete(valueType!!.rawClass)
        else
            Patchable.ignore(valueType!!.rawClass)
}

@JsonDeserialize(using = PatchableDeserializer::class)
sealed class Patchable<T> {

    class Ignore<T> : Patchable<T>()
    class Delete<T> : Patchable<T>()
    data class Update<T>(val value: T) : Patchable<T>()

    companion object {
        fun <T> update(value: T) = Update(value)
        fun <T> delete(@Suppress("UNUSED_PARAMETER") valueType: Class<T>? = null) = Delete<T>()
        fun <T> ignore(@Suppress("UNUSED_PARAMETER") valueType: Class<T>? = null) = Ignore<T>()
    }
}

The OptionalInput in #140 would easily map to our custom type.

An example of persistence is using SQL. If the provided model has a field which is 'ignore', we do not generate a SQL SET statement for that field, thereby ignoring any updates to that field.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Dec 21, 2021
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Jan 12, 2022

This looks similar to the use case in #140, but different implementation.

Thanks for the snippet. It shows how you deserialize but it's not clear how this is used? For example is Patchable for top-level arguments mainly, or is nested in some cases? You mention persisting to SQL, but Patchable can't be persisted directly via JPA and others, and requires some manual handling.

For a potential solution in more detail:

  1. To support this on top-level arguments only, it could be modelled as an API that makes it easy to check for argument presence/absence/null with the option to then convert the value to a target Object. The resulting Object can be passed to a data Repository for persistence.
  2. To support such a type nested at any level, would allow checks at any level with the caveat that the resulting Object cannot be persisted as-is, e.g. via JPA, and would require navigating the resulting Object in order to apply persistence selectively at each level.

@noogatiix
Copy link

Do you know if a native implementation of patchable/partial update is planned for the next releases of Spring Boot GraphQL ? (tell me if there is already a place listing future changes) thx 🙏

@noogatiix

This comment was marked as duplicate.

@bclozel
Copy link
Member

bclozel commented Mar 17, 2022

@noogatiix you can subscribe to this issue to learn about updates. As you can see in this conversation and linked issues, there isn't a generic approach that would work for all use cases described so far. The expected behavior, how you're combining this with other existing libraries (serialization, persistence) and the fact that this concept doesn't exist in Java - this makes it hard to find an approach that would work for all.

We're not likely to provide such a feature for the 1.0.0 version but we'll keep looking at this space.

@rstoyanchev
Copy link
Contributor

@dchan4 and @noogatiix, we've addressed this with the changes under #518. Please, take a look and provide any further comments or feedback there.

@rstoyanchev rstoyanchev closed this as not planned Won't fix, can't repro, duplicate, stale Oct 21, 2022
@rstoyanchev rstoyanchev added status: superseded Issue is superseded by another and removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged labels Oct 21, 2022
@zemanstano
Copy link

Thank you very much for addressing this issue @rstoyanchev
However, I think there may be a bug - in the original example the new isOmmited() method would still return false in both cases.

In the method org.springframework.graphql.data.GraphQlArgumentBinder#bind the variable isOmitted would correctly evaluate to true or false.

However in the method org.springframework.graphql.data.GraphQlArgumentBinder#bindRawValue there is another check for this value. If isOmitted was true, ArgumentValue.omitted() constructor is called.
The constructor is currently set to new ArgumentValue<>(null, false); - which means that in the end, the isOmmited() method will always return false.

Would it be possible to fix this?

@bclozel
Copy link
Member

bclozel commented Dec 2, 2022

@zemanstano it's really hard to follow this explanation in an issue comment. Could you share a sample project that demonstrates a concrete problem?

@rstoyanchev
Copy link
Contributor

@zemanstano I think what you're raising was addressed yesterday with #553.

@zemanstano
Copy link

@rstoyanchev yes, thank you, #553 would fix the problem I described

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: superseded Issue is superseded by another
Projects
None yet
Development

No branches or pull requests

6 participants