Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Some concern about the implementation of custom field types #1370

pmdevita started this conversation in General
Discussion options

With the merging of #1352, a long standing feature request for support for custom field types has now been solved. However, after thinking about it for a bit, I think this solution has a few problems.

  1. Global variables

The global list for type mapping was simple, but I think that's fine. However, having a function that simply edits that list is a "code smell", I know Django relies on globals a lot but this feels like a step too far, and it has some knock on effects.

  1. Load order

Because a function must be called to register a type, that function now must be called before the schema using it is initialized. I suppose since models already must be loaded first this might always be OK, but then the registration call has to live somewhere that gets loaded first. This is really a piece of configuration, so maybe it should live in settings.py? But it's a function call, not a list or dictionary...

  1. Inheritance

I think this is the biggest thing the current solution lacks. There are a number of libraries that implement custom fields that are just subclassing built-in Django fields. The current solution requires you to register each of these manually, but it would be a lot nicer if Ninja could determine this automatically. For fully custom fields that also have subclasses, these all must be manually registered too.

Additionally, because it's a function call, library authors who may wish to add support for Ninja to their library have a more difficult task. They cannot simply import that function and call it, they need to check if Ninja is an available library and they probably should also check if it's being loaded in the Django project.

With that all in mind, I think this solution #1320 had some advantages in this regard. It avoids global variables and load order issues, and provides an avenue for inheritance and library support. I would like to see some kind of hybrid solution, one that can either pull the type from the special function on the field, or introspect its class and pull a type from the mapping of built-in fields.

You must be logged in to vote

Replies: 1 comment 3 replies

Comment options

I observed for a while how people were just using TYPES global variable and it worked pretty fine..

indeed you have to "register" it somewhere before you import your model-schemas, but overall that looks easy

the problem with FIeld.get_schema_type - is that other library authors have no intention implementing it, so without register global user will have to inherit custom field and create get_schema_type - which would be annoying experience

You must be logged in to vote
3 replies
Comment options

the problem with FIeld.get_schema_type - is that other library authors have no intention implementing it, so without register global user will have to inherit custom field and create get_schema_type - which would be annoying experience

I don't think that's necessarily true, I think we're seeing Ninja emerge as the leading choice for new API projects on Django, and it's definitely not the only project that would benefit from better integration with Pydantic. But I agree, large-scale adoption of the practice would be a long time away if it even ever happened, which is why I think some kind of hybrid approach is necessary.

As I'm writing, I now think maybe it would be possible to achieve this in settings somehow. Maybe a dictionary mapping import paths to a Pydantic type? If we're going to keep this paradigm, it's really a setting and should probably live in settings.py.

For example, with something like VectorField from pgvector,

NINJA_CUSTOM_TYPES = {
 "pgvector.django.vector.VectorField": List[int]
}

This approach feels a lot more "Django" to me.

pgvector is also a great example of something that would benefit from the function though, as one would probably want to validate its dimensions and that's going to be dependent on the field's settings.

class VectorField(Field):
 ...
 def get_schema_type(self):
 from pydantic import conlist
 return conlist(int, min_length=self.dimensions, max_length=self.dimensions)

This constraint is a more accurate adaptation, but without the function, you would have to manually define this field on every schema that uses it.

Comment options

NINJA_CUSTOM_TYPES = {
 "pgvector.django.vector.VectorField": List[int]
}

Well I think this one is the least "comfortable" as it fully need user attention to configure it in the project

register_field gives library creators a way to integrate with django ninja without forcing users to configure anything (if they want to or care at all)

in fact some may even create a pip package that will collect most popular django packages which provide custom fields and register it al at once based on what's installed in system packages

get_schema_type on the other hand breaks few SOLID principles (like why would ever pgvector developers add that method - spoiling the code base for users who may not even use django ninja)

Comment options

get_schema_type on the other hand breaks few SOLID principles

Mulling that over, I think that's a very reasonable criticism of the extra class method. pgvector was maybe not the best example here though because it's also demonstrating another problem, dynamic typing based on field settings. For most field types that don't need special Pydantic extras, they can just do

def get_schema_type(self):
 return str

That's pretty simple and I think it's a lot easier to persuade library maintainers of than something like this

try:
 from ninja.orm import register_field
 register_field(MyCustomField, str)
except:
 pass

Which will always require the addition of a soft dependency on Ninja for them.

Either path is possible with third party compatibility libraries, I'd hope to see some as well. Yes, the class function is not automatic in third party libraries, but it is a cleaner solution.

register_field is a bad practice. It creates a hidden dependency wherever you place it. It treats a global mapping, which is functionally configuration, as something that could or should be updated on the fly. Users must have a full understanding of Django's import and load order to guess where it should go, and improperly placing it could create a confusing pitfall to fix it later on. I can't think of any other decently popular Django package that does something like this, much less within the broader Python ecosystem. Sure, it worked for the users who implemented it on their own, but what other choice did they have?

Using settings.py to configure this makes for a better perspective on how the mapping works. I'll cover a couple scenarios for how the settings implementation affects users and other library maintainers:

For other library maintainers, this is actually the easiest way for them to provide some kind of official support. All they have to do now is add this to their docs.

Django Ninja Support

Add this to your settings.py

NINJA_CUSTOM_TYPES = {
"mylibrary.fields.MyField": List[int]
}

Yes they could have done this with register_field but they then either have to explain its pitfalls with placement or omit that for the user to discover themselves. This is much easier for users, particularly beginners, to get right.

For Ninja users who wanted this functionality but their library had no intention to add support, they already had to give this problem "full user attention". The effort involved has not increased, but the end result is far more explicit and maintainable.

For Ninja users who wanted this functionality and their library could have used register_field, yes they will have to add it manually. While the concern that users (who previously had to do nothing) are now "forced" to add configuration is understandable, this gives a proper and explicit place for configuring this functionality, and it is an established pattern Django users are used to seeing. Additionally, if limited library adoption is expected anyways, then this scenario is far more unlikely and less concerning.

I think a compromise could be made here to have both - maybe we do leave in register_field for library maintainers if they so wish, and users can benefit from the better organization of the settings.py approach.

I'm not sure there is a perfect solution, there's a funny intersection of problems here. I'm also not particularly attached to any of my solutions and I'm open to more ideas. Inheritance would be a really clean way to solve this, but that does perhaps place unreasonable expectations on Django and other libraries. I think as long as we're stuck with a global mapping approach though, framing it as configuration is the best way to present the solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants

AltStyle によって変換されたページ (->オリジナル) /