-
-
Notifications
You must be signed in to change notification settings - Fork 528
-
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.
- 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.
- 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...
- 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.
Beta Was this translation helpful? Give feedback.
All reactions
Replies: 1 comment 3 replies
-
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
Beta Was this translation helpful? Give feedback.
All reactions
-
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.
Beta Was this translation helpful? Give feedback.
All reactions
-
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)
Beta Was this translation helpful? Give feedback.
All reactions
-
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.
Beta Was this translation helpful? Give feedback.
All reactions
-
👍 1