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

Type-safe "optional-nullable" fields #3779

Open
1 of 3 tasks
nrbnlulu opened this issue Feb 12, 2025 · 7 comments · May be fixed by #3791
Open
1 of 3 tasks

Type-safe "optional-nullable" fields #3779

nrbnlulu opened this issue Feb 12, 2025 · 7 comments · May be fixed by #3791

Comments

@nrbnlulu
Copy link
Member

nrbnlulu commented Feb 12, 2025

Preface

it is a common practice in strawberry that when your data layer have
an optional field i.e

class Person:
    name: str
    phone: str | None = None

and you want to update it you would use UNSET in the mutation input
in order to check whether this field provided by the client or not like so:

@strawberry.input
class UpdatePersonInput:
    id: strawberry.ID
    name: str| None
    phone: str | None = UNSET

@strawberry.mutation
def update_person(input: UpdatePersonInput) -> Person:
    inst = service.get_person(input.id)
    if name := input.name:
         inst.name = name
    if input.phone is not UNSET:
        inst.phone = input.phone  # ❌ not type safe
    
    service.save(inst)

Note that this is not an optimization rather a business requirement.
if the user wants to nullify the phone it won't be possible other wise
OTOH you might nullify the phone unintentionally.

This approach can cause lots of bugs since you need to remember that you have
used UNSET and to handle this correspondingly.

Since strawberry claims to

Strawberry leverages Python type hints to provide a great developer experience while creating GraphQL Libraries.

it is only natural for us to provide a typesafe way to mitigate this.

Proposal

The Option type.which will require only this minimal implementation

import dataclasses


@dataclasses.dataclass
class Some[T]:
    value: T
    
    def some(self) -> Some[T | None] | None:
      return self
    
@dataclasses.dataclass
class Nothing[T]:
    def some(self) -> Some[T | None] | None:
      return None 

Maybe[T] = Some[T] | Nothing[T]

and this is how you'd use it

@strawberry.input
class UpdatePersonInput:
    id: strawberry.ID
    name: str| None
    phone: Maybe[str | None]

@strawberry.mutation
def update_person(input: UpdatePersonInput) -> Person:
    inst = service.get_person(input.id)
    if name := input.name:
         inst.name = name
    if phone := input.phone.some(): 
        inst.phone = phone.value  # ✅  type safe
    
    service.save(inst)

Currently if you want to know if a field was provided

Backward compat

UNSET can remain as is for existing codebases.
Option would be handled separately.

which Option library should we use?

  1. Don't use any library craft something minimal our own as suggested above.
  2. ** use something existing**

The sad truth is that there are no well-maintained libs in the ecosystem.

Never the less it is not hard to maintain something just for strawberry since the implementation
is rather straight forward and not much features are needed. we can fork either

and just forget about it.

  1. allow users to decide
# before anything

strawberry.register_option_type((MyOptionType, NOTHING))

then strawberry could use that and you could use whatever you want.

  • Core functionality
  • Alteration (enhancement/optimization) of existing feature(s)
  • New behavior
@fruitymedley
Copy link

To clarify, is the following the use case you're interested in?

mutation Update {
    updatePerson(  ## this operation sets phone to null
        input: {
            id: ..., 
            name: ...,
            phone: null
        }
    ) {
        ...
    }
}

vs

mutation Update {
    updatePerson(  ## this operation does not modify phone number
        input: {
            id: ..., 
            name: ...
        }
    ) {
        ...
    }
}

@nrbnlulu
Copy link
Member Author

yep

@Speedy1991
Copy link
Contributor

I think this is a solid proposal. Both Option and Maybe are valid choices, though Maybe seems to be slightly better maintained. However, I’m not entirely sure if this justifies adding a third-party library as a dependency.

For an opt-in approach, I would prefer your solution:

strawberry.register_option_type((MyOptionType, NOTHING))

If this Type is meant to be included directly in Strawberry, I would avoid forking the repository. Instead, I’d suggest copying the relevant MIT-licensed code while ensuring proper attribution, such as adding credits in the form of code comments or including a reference to the original license.

@nrbnlulu
Copy link
Member Author

@patrick91 👀 ?

@patrick91
Copy link
Member

Code sample in pyright playground

from __future__ import annotations
import dataclasses
from typing import Self, reveal_type


@dataclasses.dataclass
class Some[T]:
    value: T
    
    def some(self) -> Self:
      return self
    
@dataclasses.dataclass
class Nothing[T]:
    def some(self) -> None:
      return None 

type Maybe[T] = Some[T | None] | Nothing[T]

def some_func() -> Maybe[str]:
    return Some("a")

a = some_func()

reveal_type(a)

if phone := a.some():
    reveal_type(phone)
    reveal_type(phone.value)
else:
    reveal_type(phone)

@Corentin-Bravo
Copy link

I have met the exact same problem, but I don't understand why you cannot solve it.

@strawberry.input
class UpdatePersonInput:
    id: strawberry.ID
    name: str| None
    phone: str | None | UnsetType = strawberry.field(
        graphql_type=str | None,
        default=UNSET
    )

@strawberry.mutation
def update_person(input: UpdatePersonInput) -> Person:
    inst = service.get_person(input.id)
    if name := input.name:
         inst.name = name
    if not instance(input.phone, UnsetType):
        inst.phone = phone.value
    
    service.save(inst)

The case where you do not want None but either a string or nothing cannot be handled cleanly due to the GraphQL specification (an optional value is a nullable value**), but if your API is used only internally, you can do the same thing:

@strawberry.input
class UpdatePersonInput:
    id: strawberry.ID
    name: str| None
    phone: str  | UnsetType = strawberry.field(
        graphql_type=str | None,
        default=UNSET
      description="This value is optional but not nullable, do not send a null!"
    )

(Arguably you could write custom code to ensure that sending a None would return an appropriate error instead of randomly breaking inside your code)

I find the proposition slightly contrived, as it turns scalar values into objects.

@nrbnlulu
Copy link
Member Author

nrbnlulu commented Feb 23, 2025

@Corentin-Bravo

phone: str | None | UnsetType

can actually solve this, I haven't thought about this really...

Though:

  1. nothing stops you from using UNSET on anything.
    UNSET is UNSET: Any = UnsetType()
    because how we advertised the use of unset up until now which is
phone: str | None = strawberry.UNSET

You could say that we can change the signature to the real type but that would type-checkers would lint
for existing code and would require modifications.

  1. using isinstance is not very pleasant to use IMO. (prob could avoid that with a typegaurd)

We could also think re-introducing the Unset usage which could look like this

type Maybe[T] = UnsetType | None | T

def is_unset[T](val: T | UnsetType) -> TypeGuard[UnsetType]:
    return isinstance(val, UnsetType)


@strawberry.input
class UpdatePersonInput:
    phone: Maybe[str]

def update_person(input_: UpdatePersonInput) -> ...:
       if not strawberry.is_unset(input_.phone)
           update_phone(input_.phone)  

I actually have no oppositions to this.

nrbnlulu added a commit to nrbnlulu/strawberry that referenced this issue Feb 23, 2025
@sourcery-ai sourcery-ai bot linked a pull request Feb 23, 2025 that will close this issue
11 tasks
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 a pull request may close this issue.

5 participants