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

SchemaGenerator.registry refers to Dataclass not to DataclassSerializer in hooks #1343

Open
HansAarneLiblik opened this issue Dec 5, 2024 · 4 comments

Comments

@HansAarneLiblik
Copy link

Describe the bug
We have POSTPROCESSING hook, that changes the schema, based on the serializer. We're using our own @extend_schema_serializer decorator to add title to the generated schema. This implementation is copied from the original hook, as sadly title support is not available in the original.

In the hook we're

  • iterating over components.schemas
  • Retrieving the actual component from SchemaGenerator.registry
  • Accessing the "serializer" via component.object

After upgrading from 0.27.2 to 0.28.0, it seems that this does not work for DataclassSerializers anymore, as the component.object now refers to the actual Dataclass, while it used to refer to the DataclassSerializer, which we had decorated and customized

I can make a repro case if the problem is not obvious, but that takes some time.

@HansAarneLiblik
Copy link
Author

Example code

@dataclass
class A:
    field_a: int


class B(models.Model):
    field_b = models.CharField(max_length=10)


class ASerializer(DataclassSerializer[A]):
    class Meta:
        dataclass = A


class BSerializer(ModelSerializer[B]):
    class Meta:
        model = B
        fields = "__all__"


@extend_schema(responses=ASerializer)
@api_view()
def a_view(request: Request) -> Response:
    pass


@extend_schema(responses=BSerializer)
@api_view()
def b_view(request: Request) -> Response:
    pass

# hook
def test_hook(result, generator, **_kwargs) -> Any:
    for name, schema in result["components"]["schemas"].items():
        component_key: tuple[str, str] = (name, ResolvedComponent.SCHEMA)
        try:
            component: ResolvedComponent = generator.registry[component_key]
        except KeyError:
            continue

        print(component)

    return result

registry[component_key].object is Serializer in 0.27.2

image

Same object in 0.28.0

image

@tfranzel
Copy link
Owner

tfranzel commented Dec 5, 2024

Hi,

After upgrading from 0.27.2 to 0.28.0, it seems that this does not work for DataclassSerializers anymore, as the component.object now refers to the actual Dataclass, while it used to refer to the DataclassSerializer, which we had decorated and customized

So this was actually a slightly incorrect implementation that lead to collisions in specific cases: #1288

We introduced a new mechanism to better identify the component with #1290. In case of DataclassSerializer, this is a way better representation of what is going on.

It looks like this bugfix broke your modified code due to dependence on changing internals in spectacular. What you used there is not really a public API. There is really nothing to fix here on our side I believe.

However, I believe this should be rather easy to fix on your side.

  • Just create a subclass of ComponentIdentity with an extra param for the serializer.
  • Subclass the dataclasses plugin
    • give it a higher priority so it takes precedence
    • just override the new method like this:
       def get_identity(self, auto_schema, direction: Direction) -> Any:
           return YourComponentIdentity(self.target.dataclass_definition.dataclass_type, self.target)
    • Use this in your custom postprocessing hook instead

tldr: the custom ComponentIdentity is not strictly needed. You could just bolt on the serializer via assignment. In that case you just need the dataclass extension subclass with that one method override.

@HansAarneLiblik
Copy link
Author

But, when accessing django.db.Model components like this, they still refer to the ModelSerializer instead of Model itself.

But I'll take a look at your suggestion! Thanks!

@tfranzel
Copy link
Owner

tfranzel commented Dec 5, 2024

But, when accessing django.db.Model components like this, they still refer to the ModelSerializer instead of Model itself.

thats true, but they also behave differently and so there is no need for that indirection. Please look at #1288 and it will make sense.

The new system is optional and only used when needed. However, dataclasses needs it to generate correct schema without false positive warnings.

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

No branches or pull requests

2 participants