vitalik / django-ninja

💨 Fast, Async-ready, Openapi, type hints based framework for building APIs
https://django-ninja.dev
MIT License
7.38k stars 438 forks source link

[BUG] Allow specifying field types for custom fields #1109

Open scott-8 opened 8 months ago

scott-8 commented 8 months ago

Describe the bug Django-ninja uses get_internal_type to determine field type for schemas. This works with all of the default fields, but does not work with custom fields. For fields that don't define it, Django will return self.__class__.__name__. For fields that define a custom get_internal_type or db_type, the type parsed by django-ninja might not necessarily be correct.

For example, I have a custom field that stores data in the database as a string, but converts that string to a bool when the data is read from the database. Since get_internal_type returns CharField, django-ninja generates a schema with string, while ideally it would be a bool.

This is fixable by adding annotations for schemas using these custom fields, but it would be nice to have a solution on the field level. The code in get_schema_field gets the field type like this:

internal_type = field.get_internal_type()
python_type = TYPES[internal_type]

I'd propose changing it to this:

if hasattr(field, "get_python_type"):
    python_type = field.get_python_type()
else:
    internal_type = field.get_internal_type()
    python_type = TYPES[internal_type]

That would let a user define get_python_type on the custom field. If this is something you'd be interested in I can make the change.

Versions (please complete the following information):

RobertKolner commented 1 month ago

I agree, this is an elegant solution and I've come to the exact same conclusion in my own tests. One complementary bit of code would be a function to register a new type in TYPES. It's of course possible to do this with a small monkey-patch, but it would be much better to have this be a supported.

My current solution to this exact problem is something similar to the following:

# A decorator that registers a schema for a given type
def register_in_ninja_for_db_field(db_field_internal_type: str):
    def decorator(klass):
        ninja.orm.fields.TYPES[db_field_internal_type] = klass
        return klass
    return decorator

...

class MyJsonField(models.JSONField):
    ...
    def get_internal_type(self):
        return "MyJsonField"

    def db_type(self, connection):
        return "jsonb"  # Only supports Postgres. It's possible to do this in a more proper way by copy-pasting code from django.db.models.fields. It also does nothing about 'db_check', 'cast_db_type', 'db_type_suffix' which also use 'get_internal_type'

@register_in_ninja_for_db_field("MyJsonField")
class MyJsonFieldSchema(Schema):
    name: str
    age: int

With the solution proposed by Scott there would be no need to override get_internal_type and db_type, which results in much more reliable definition.