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

Allow abstract subclasses of ModelSchema #63

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 9 additions & 0 deletions djantic/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,9 @@ def __new__(
f"{exc} (Is `Config` class defined?)"
)

if getattr(config, "abstract", False):
continue

include = getattr(config, "include", None)
exclude = getattr(config, "exclude", None)

Expand Down Expand Up @@ -165,6 +168,12 @@ class ModelSchema(BaseModel, metaclass=ModelSchemaMetaclass):
class Config:
orm_mode = True

def __init__(self, **data):
if getattr(self.Config, "abstract", False):
raise ConfigError("Abstract ModelSchema cannot be instantiated.")

super().__init__(**data)

@classmethod
def schema_json(
cls,
Expand Down
27 changes: 27 additions & 0 deletions docs/usage.md
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,33 @@ class UserSchema(ModelSchema):

Once defined, the `UserSchema` can be used to perform various functions on the underlying Django model object, such as generating JSON schemas or exporting serialized instance data.

### Custom subclasses
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes:

  1. Make this heading ### Abstract schema models
  2. Place as the next heading after Customizing the schema


Abstract subclasses can be defined to implement methods shared over for a number of ModelSchemata, note that they cannot be instantiated by themselves.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename ModelSchema.


```python
from typing import Optional
from django.db.models import Model
from djantic import ModelSchema
from myapp.models import User

class BaseModelSchema(ModelSchema):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make more obvious that this is an example and what the use-case is? This may be misinterpreted of how to define all abstract schema. For example, the model: Optional[Model] = None isn't required, but not clear.

class Config:
model: Optional[Model] = None
abstract = True

def to_django(self) -> Model:
if self.Config.model is not None:
return self.Config.model.objects.create(**self.dict())
raise NotImplementedError()

class CreateUserSchema(BaseModelSchema):
class Config:
model = User

```


### Basic schema usage

The `UserSchema` above can be used to generate a JSON schema using Pydantic's [schema](https://pydantic-docs.helpmanual.io/usage/schema/) method:
Expand Down
2 changes: 1 addition & 1 deletion poetry.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

17 changes: 17 additions & 0 deletions tests/test_main.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,23 @@ class Config:
include = ["id"]
exclude = ["first_name"]

class AbstractModelSchema(ModelSchema):
class Config:
abstract = True
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is expected to happen when model, include, or exclude are set on the config? Need coverage for these cases.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very good point, I'll ping you once I have a minute to look into it.

In my opinion none of those should be present on an abstract model, as its main raison d'etre is to reduce boilerplate and allow proper typing in codebases where multiple ModelSchemata are used in a similar customised manner. Do you have any thoughts on the matter as the repo/package owner?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it would be best if we started out with some warnings (raise config error) if someone tries to include these on the abstract model to avoid confusion, then if someone later presented a valid use-case to support any of these then maybe could do then if it made sense.


with pytest.raises(
ConfigError,
match="Abstract ModelSchema cannot be instantiated.",
):
schema = AbstractModelSchema()

class InheritedSchema(AbstractModelSchema):
class Config:
model = User
include = ["id"]

schema = InheritedSchema(id=1)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add specific assertions.



@pytest.mark.django_db
def test_get_field_names():
Expand Down