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

default_factory doesn't work #3517

Open
ShtykovaAA opened this issue May 27, 2024 · 11 comments
Open

default_factory doesn't work #3517

ShtykovaAA opened this issue May 27, 2024 · 11 comments
Labels
bug Something isn't working
Milestone

Comments

@ShtykovaAA
Copy link

ShtykovaAA commented May 27, 2024

Hi!
I use default_factory to initialize my variable, but variable always returns the same result. It seems like default_factory doesn't work and it returns always the same result of function.

Here is example to reproduce:
https://play.strawberry.rocks/?gist=a7a5e62ffe4e68696b44456398d11104

Upvote & Fund

  • We're using Polar.sh so you can upvote and help fund this issue.
  • We receive the funding once the issue is completed & confirmed by you.
  • Thank you in advance for helping prioritize & fund our backlog.
Fund with Polar
@ShtykovaAA ShtykovaAA added the bug Something isn't working label May 27, 2024
@patrick91
Copy link
Member

I can reproduce this, I think it's because we store the default value when creating the object, see:

CleanShot 2024-05-28 at 16 35 47@2x

@patrick91
Copy link
Member

Fixing this will potentially be a breaking change 🤔

@coady what do you think of this issue?

@coady
Copy link
Member

coady commented May 28, 2024

I don't think this can be supported (and still be compliant with the spec). GraphQL defaults are statically in the schema. strawberry export-schema ...:

type Mutation {
  update1(fields: MyInputType!): String!
  update2(fields: MyInputType!): String!
}

input MyInputType {
  field1: String = "318fbf6e-73b6-40eb-932f-0b66ba935b75"
}

type Query {
  hello: String!
}

Another issue is that the client sending an explicit null is valid and semantically different. So MyInputType would need logic like

if field1 in (None, UNSET):
    field1 = uuid_pkg.uuid4()

That may seem like a workaround, but is actually the only correct implementation.

@ShtykovaAA
Copy link
Author

@patrick91 @coady hi!

Thanks for your answers. Yes, now it's the only way with None, UNSET.

The main reason why I'm thinking about another implementation is pydantic models, I really like to use @strawberry.experimental.pydantic.input and when I inherit a model with default_factory, then this default_factory is inherited in the field too and I can't override this field with UNSET and None

example:

import strawberry
from pydantic import BaseModel, field

class PydanticModel(BaseModel):
    field1: str | None = Field(default_factory=uuid_pkg.uuid4)

@strawberry.experimental.pydantic.input(PydanticModel)
class MyInputType():
    field1: str | None = strawberry.UNSET # not works here

By the way I sent daft PR with overriding #3469

@ShtykovaAA
Copy link
Author

@patrick91 and also can I ask in what situations then we need to use default_factory if this field is static, when we can use only default?

@patrick91
Copy link
Member

@patrick91 and also can I ask in what situations then we need to use default_factory if this field is static, when we can use only default?

I'm not sure to be honest, I'll need to think about this a bit

I do think it might be a flaw, or at least something surprising, so maybe we need to reconsider it

@bellini666
Copy link
Member

I do understand the schema's default value issue, but I do agree with this comment. I actually had to do workaround a similar issue on strawberry-resources when exporting form data for the field as a dynamic default value would not actually make sense there.

So my vote would be to actually change the behavior to fix this issue, specially since we still are 0.x =P, and mention as a "possible breaking change" in the changelog, mentioning the use of default as the correct way of relying on the older behavior

@patrick91
Copy link
Member

patrick91 commented May 29, 2024

just throwing out some ideas to make the change less painful

  1. we could a static_factory or schema_factory which will have the current behaviour, default_factory will mimic dataclasses' and pydantic's behaviour
  2. have a configuration option to disable static defaults
  3. just change the behaviour

option 1 could also have a codemod to make the update easier 😊

@patrick91
Copy link
Member

@coady sorry to ping you again, but do you use default_factory for defaults in the schema level? 😊 what's your use case exactly?

@coady
Copy link
Member

coady commented May 29, 2024

@coady sorry to ping you again, but do you use default_factory for defaults in the schema level? 😊 what's your use case exactly?

The only use I'm aware of (and use) is for mutables, as dataclasses requires. Any valid value is a valid default value, including [] and {}.

I'd be happy with a cleaner alternative for mutables. This is forbidden (but is valid GraphQL):

    q: list[float] = [0.5]

So instead I have to use default_factory or this:

    q: list[float] = (0.5,)  # type: ignore

which mypy complains about.

@patrick91
Copy link
Member

just throwing out some ideas to make the change less painful

  1. we could a static_factory or schema_factory which will have the current behaviour, default_factory will mimic dataclasses' and pydantic's behaviour
  2. have a configuration option to disable static defaults
  3. just change the behaviour

option 1 could also have a codemod to make the update easier 😊

I think we could do option 2 for the time being

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants