strawberry icon indicating copy to clipboard operation
strawberry copied to clipboard

default_factory doesn't work

Open ShtykovaAA opened this issue 1 year ago • 11 comments

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

ShtykovaAA avatar May 27 '24 15:05 ShtykovaAA

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 avatar May 28 '24 15:05 patrick91

Fixing this will potentially be a breaking change 🤔

@coady what do you think of this issue?

patrick91 avatar May 28 '24 15:05 patrick91

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.

coady avatar May 28 '24 20:05 coady

@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 https://github.com/strawberry-graphql/strawberry/pull/3469

ShtykovaAA avatar May 29 '24 12:05 ShtykovaAA

@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?

ShtykovaAA avatar May 29 '24 12:05 ShtykovaAA

@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

patrick91 avatar May 29 '24 12:05 patrick91

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

bellini666 avatar May 29 '24 14:05 bellini666

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 avatar May 29 '24 16:05 patrick91

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

patrick91 avatar May 29 '24 16:05 patrick91

@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.

coady avatar May 29 '24 16:05 coady

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

patrick91 avatar Jun 03 '24 10:06 patrick91