fix: properly resolve dataclass forward references

[guacs]

Pull Request Checklist

• New code has 100% test coverage
• (If applicable) The prose documentation has been updated to reflect the changes introduced by this PR
• (If applicable) The reference documentation has been updated to reflect the changes introduced by this PR
• Pre-Commit Checks were ran and passed
• Tests were ran and passed

Description

This PR reverts #371 because the dataclasses fields API does not properly resolve the forward annotations as explained in this issue. For getting the resolved types, we have to use get_type_hints though that doesn't work well with models like MappedAsDataclass of sqlalchemy. For those cases, it's probably better for users to subclass the current DataclassFactory implementation and handle them accordingly.

Close Issue(s)

• Fixes Bug: forward referenced nested models don't work with DataclassFactory #382

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

[peterschutt]

Should #347 be re-opened?

[guacs]

Should #347 be re-opened?

Yeah I think so. For fixing these kinds of issues, I was thinking maybe we could have a get_type_hints class method that can be overridden. This way for special cases like MappedAsDataclass, the users could override it to return the correct type hints. What do you think about this approach @peterschutt?

[peterschutt]

What do you think about this approach @peterschutt?

I have to admit I'm not very familiar with polyfactory, so overall I'm inclined to follow your lead.

I did take a quick look though and one thing that stands out is that each of the get_model_fields() implementations for each of the factory types all directly instantiate FieldMeta instances, like:

            fields_meta.append(
                FieldMeta.from_type(
                    annotation=annotation,
                    name=field.name,
                    default=default_value,
                    random=cls.__random__,
                    randomize_collection_length=cls.__randomize_collection_length__,
                    min_collection_length=cls.__min_collection_length__,
                    max_collection_length=cls.__max_collection_length__,
                )
            )

Perhaps you could consider adding a create_field_meta() classmethod onto BaseFactory, something like:

class BaseFactory:
    ...

    @classmethod
    def create_field_meta(
        cls,
        annotation, 
        name, 
        default, 
        random=None, 
        randomize_collection_length=None, 
        min_collection_length=None,
        max_collection_length=None,
    ) -> FieldMeta:
        return FieldMeta.from_type(
            annotation=annotation,
            name=name,
            default=default_value,
            random=random or cls.__random__,
            randomize_collection_length=randomize_collection_length or cls.__randomize_collection_length__,
            min_collection_length=min_collection_length or cls.__min_collection_length__,
            max_collection_length=max_collection_length or cls.__max_collection_length__,
        )

This would clean up the repetition of passing random=cls.__random__ and the others to the constructor each time, e.g., the original example above would become:

            fields_meta.append(
                cls.create_field_meta(
                    annotation=annotation,
                    name=field.name,
                    default=default_value,
                )
            )

And would also provide a convenient place for sub-classes to intercept the annotation:

class SQLAlchemyDCFactory(DataclassFactory):
    @classmethod
    def create_field_meta(cls, annotation, *args, **kwargs) -> FieldMeta:
        annotation = unwrap_mapped_annotation(annotation)
        return super().create_field_meta(annotation, *args, **kwargs)

WDYT?

[guacs]

I'm really like the idea of allowing the users to override the creation of the FieldMeta, but I'm not sure I'm a fan of the create_field_meta on the factory class. The way the overriding of the FieldMeta has been done so far is by subclassing the FieldMeta as seen in the PydanticFieldMeta. So, I think it might be nice if we instead allow users to provide a __field_meta_cls__ on the factory which can then be used to create the FieldMeta. So essentially the calls will become cls.__field_meta_cls__.from_type.

[guacs]

I'm going to merge this now and reopen that issue. I think we can move the discussion into there.

[peterschutt]

I'm really like the idea of allowing the users to override the creation of the FieldMeta, but I'm not sure I'm a fan of the create_field_meta on the factory class. The way the overriding of the FieldMeta has been done so far is by subclassing the FieldMeta as seen in the PydanticFieldMeta. So, I think it might be nice if we instead allow users to provide a __field_meta_cls__ on the factory which can then be used to create the FieldMeta. So essentially the calls will become cls.__field_meta_cls__.from_type.

There's a argument to do both, even if create_field_meta() is made private _create_field_meta() so that modifying the meta class is the "one way" to do it. The argument being reducing dupliction of:

            random=random or cls.__random__,
            randomize_collection_length=randomize_collection_length or cls.__randomize_collection_length__,
            min_collection_length=min_collection_length or cls.__min_collection_length__,
            max_collection_length=max_collection_length or cls.__max_collection_length__,

So it would be:

class BaseFactory:
    ...

    @classmethod
    def _create_field_meta(
        cls,
        annotation, 
        name, 
        default, 
        random=None, 
        randomize_collection_length=None, 
        min_collection_length=None,
        max_collection_length=None,
    ) -> FieldMeta:
        return cls.__field_meta_cls__.from_type(
            annotation=annotation,
            name=name,
            default=default_value,
            random=random or cls.__random__,
            randomize_collection_length=randomize_collection_length or cls.__randomize_collection_length__,
            min_collection_length=min_collection_length or cls.__min_collection_length__,
            max_collection_length=max_collection_length or cls.__max_collection_length__,
        )

However we are getting into the realms of personal preference now, so I'll back your decision from here 👍

[guacs]

Hm...I do understand your point about the reducing the duplication of passing the arguments to the field type, but I don't feel like it provides enough of an advantage that it warrants another layer of indirection/abstraction. However, I definitely liked your idea of allowing users to capture the annotation and override it in some if they wanted to. I think we should go ahead with the __field_meta_cls__ option.