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.

Sign up for GitHub

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

style: format code with Black, isort and Ruff Formatter #10

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

style: format code with Black, isort and Ruff Formatter #10

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions django_ninja_crudl/crudl.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
from typing import TYPE_CHECKING, Any, ClassVar, Literal, cast
from uuid import UUID

from django2pydantic import Infer, ModelFields
from django2pydantic.schema import Schema
from django.core.exceptions import ValidationError
from django.db import IntegrityError, models, transaction
from django.db.models import (
Expand All @@ -30,8 +32,6 @@
)
from pydantic import BaseModel

from django2pydantic import Infer, ModelFields
from django2pydantic.schema import Schema
from django_ninja_crudl.base import CrudlBaseMixin
from django_ninja_crudl.errors.openapi_extras import (
not_authorized_openapi_extra,
Comment on lines 32 to 37
Copy link

Choose a reason for hiding this comment

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

Consider improving the naming of not_authorized_openapi_extra and throttle_openapi_extra to more descriptive names that convey their purpose more clearly. For example, openapi_extra_unauthorized_response and openapi_extra_throttle_response might provide immediate context about their usage in OpenAPI documentation enhancements.

Expand Down Expand Up @@ -126,7 +126,6 @@ def __new__(cls, name: str, bases: tuple[type, ...], dct: dict[str, Any]) -> typ

model_class: type[Model] = meta.model_class


api_meta = getattr(model_class, "CrudlApiMeta", meta)
if api_meta is None:
msg = f"CrudlApiMeta is required for model '{name}' or in the model itself."
Copy link

Choose a reason for hiding this comment

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

The error message for a missing CrudlApiMeta is not very descriptive. Consider enhancing the message to explain what CrudlApiMeta should contain or why it is essential. For example: CrudlApiMeta is required for model '{name}' or in the model itself to define necessary API metadata such as fields and permissions. This would help developers understand the error context better and resolve it more efficiently.

Expand Down
16 changes: 12 additions & 4 deletions django_ninja_crudl/errors/schemas.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,15 +38,21 @@ class ResourceNotFound404Schema(ErrorSchema):
"""The default resource not found schema."""

code: str = "ResourceNotFound"
message: str = "The requested resource was not found or you do not have permission to access it."
user_friendly_message: str = "The requested resource was not found or you do not have permission to access it."
message: str = (
"The requested resource was not found or you do not have permission to access it."
)
user_friendly_message: str = (
"The requested resource was not found or you do not have permission to access it."
Comment on lines +41 to +45
Copy link

Choose a reason for hiding this comment

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

The message and user_friendly_message in ResourceNotFound404Schema are identical, which introduces redundancy. This could be simplified to improve maintainability and reduce potential confusion.

Recommended Solution:
Consider using a single message attribute or ensure that the user_friendly_message provides a simplified or different perspective that is more accessible to end-users.

)


class Conflict409Schema(ErrorSchema):
"""The default conflict schema."""

code: str = "Conflict"
message: str = "The request could not be completed due to a conflict with the current state of the resource."
message: str = (
"The request could not be completed due to a conflict with the current state of the resource."
)

Copy link

Choose a reason for hiding this comment

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

The Conflict409Schema class lacks a user_friendly_message, which is essential for providing a better user experience by explaining the error in a non-technical manner.

Recommended Solution:
Add a user_friendly_message that simplifies the technical message to help end-users understand the nature of the conflict without needing deep technical knowledge.


class UnprocessableEntity422Schema(ErrorSchema):
Expand All @@ -73,7 +79,9 @@ class ServiceUnavailable503Schema(ErrorSchema):
"""The default service unavailable schema."""

code: str = "ServiceUnavailable"
message: str = "The server is currently unable to handle the request due to a temporary overloading or maintenance of the server."
message: str = (
"The server is currently unable to handle the request due to a temporary overloading or maintenance of the server."
)
user_friendly_message: str = (
"The server is currently unavailable. Please try again later."
)
141 changes: 107 additions & 34 deletions tests/test_django/app/migrations/0001_initial.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@


class Migration(migrations.Migration):

initial = True

dependencies = [
Expand All @@ -15,66 +14,140 @@ class Migration(migrations.Migration):

operations = [
migrations.CreateModel(
name='Author',
name="Author",
fields=[
('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
('name', models.CharField(max_length=100)),
('birth_date', models.DateField()),
(
"id",
models.BigAutoField(
auto_created=True,
primary_key=True,
serialize=False,
verbose_name="ID",
),
),
("name", models.CharField(max_length=100)),
("birth_date", models.DateField()),
],
),
migrations.CreateModel(
name='Library',
name="Library",
fields=[
('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
('name', models.CharField(max_length=100)),
('address', models.TextField()),
(
"id",
models.BigAutoField(
auto_created=True,
primary_key=True,
serialize=False,
verbose_name="ID",
),
),
("name", models.CharField(max_length=100)),
("address", models.TextField()),
],
),
migrations.CreateModel(
name='Publisher',
name="Publisher",
fields=[
('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
('name', models.CharField(max_length=100)),
('address', models.TextField(help_text="Publisher's official address")),
(
"id",
models.BigAutoField(
auto_created=True,
primary_key=True,
serialize=False,
verbose_name="ID",
),
),
("name", models.CharField(max_length=100)),
("address", models.TextField(help_text="Publisher's official address")),
],
),
migrations.CreateModel(
name='Book',
name="Book",
fields=[
('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
('title', models.CharField(max_length=200)),
('isbn', models.CharField(max_length=13, unique=True)),
('publication_date', models.DateField()),
('authors', models.ManyToManyField(to='app.author')),
('publisher', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='app.publisher')),
(
"id",
models.BigAutoField(
auto_created=True,
primary_key=True,
serialize=False,
verbose_name="ID",
),
),
("title", models.CharField(max_length=200)),
("isbn", models.CharField(max_length=13, unique=True)),
("publication_date", models.DateField()),
("authors", models.ManyToManyField(to="app.author")),
(
"publisher",
models.ForeignKey(
on_delete=django.db.models.deletion.CASCADE, to="app.publisher"
),
),
],
options={
'default_related_name': 'books',
"default_related_name": "books",
},
),
migrations.CreateModel(
name='BookCopy',
name="BookCopy",
fields=[
('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
('inventory_number', models.CharField(max_length=20, unique=True)),
('book', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='app.book')),
('library', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='app.library')),
(
"id",
models.BigAutoField(
auto_created=True,
primary_key=True,
serialize=False,
verbose_name="ID",
),
),
("inventory_number", models.CharField(max_length=20, unique=True)),
Comment on lines +77 to +103
Copy link

Choose a reason for hiding this comment

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

Unique Constraints on Fields

The fields isbn in the Book model and inventory_number in the BookCopy model are marked as unique. While this is generally a good practice for data integrity, it can lead to performance issues if these fields are frequently queried but not properly indexed. Consider adding explicit database indexes to these fields to improve query performance.

Recommendation:
Add indexes using models.Index(fields=['isbn'], name='idx_isbn') for the Book model and models.Index(fields=['inventory_number'], name='idx_inventory_number') for the BookCopy model to ensure efficient lookups.

(
"book",
models.ForeignKey(
on_delete=django.db.models.deletion.CASCADE, to="app.book"
),
),
(
"library",
models.ForeignKey(
on_delete=django.db.models.deletion.CASCADE, to="app.library"
),
),
],
options={
'default_related_name': 'book_copies',
"default_related_name": "book_copies",
},
),
migrations.CreateModel(
name='Borrowing',
name="Borrowing",
fields=[
('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
('borrow_date', models.DateField()),
('return_date', models.DateField(blank=True, null=True)),
('book_copy', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='app.bookcopy')),
('user', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to=settings.AUTH_USER_MODEL)),
(
"id",
models.BigAutoField(
auto_created=True,
primary_key=True,
serialize=False,
verbose_name="ID",
),
),
("borrow_date", models.DateField()),
("return_date", models.DateField(blank=True, null=True)),
(
"book_copy",
models.ForeignKey(
on_delete=django.db.models.deletion.CASCADE, to="app.bookcopy"
),
),
(
"user",
models.ForeignKey(
on_delete=django.db.models.deletion.CASCADE,
Comment on lines +83 to +144
Copy link

Choose a reason for hiding this comment

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

Cascade Delete in Foreign Key Relationships

The foreign key relationships in the Book, BookCopy, Borrowing models use CASCADE as the deletion rule. This means that deleting a record in the parent table (Publisher, Book, Library, BookCopy, User) will automatically delete all associated records in the child model. This can lead to unintentional data loss if not handled carefully.

Recommendation:
Review the business logic to ensure that cascade deletion is intended. If not, consider changing the deletion rule to PROTECT or SET_NULL to prevent accidental data deletions.

to=settings.AUTH_USER_MODEL,
),
),
],
options={
'default_related_name': 'borrowings',
"default_related_name": "borrowings",
},
),
]
2 changes: 1 addition & 1 deletion tests/test_django/app/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@

from typing import ClassVar, override
Copy link

Choose a reason for hiding this comment

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

The import from typing import ClassVar, override seems to incorrectly include override, which is not part of the Python typing module. This might be a typo or a misunderstanding of the module's capabilities. If override is intended to be used as a decorator for method overriding, ensure it is imported from the correct library or framework that provides this functionality, or remove it if it's not applicable.


from django2pydantic import Infer, ModelFields
from django.contrib.auth.models import User
from django.db import models

from django2pydantic import Infer, ModelFields
from django_ninja_crudl.crudl import CrudlApiBaseMeta


Expand Down
2 changes: 1 addition & 1 deletion tests/test_django/wsgi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,6 @@

from django.core.wsgi import get_wsgi_application

os.environ.setdefault('DJANGO_SETTINGS_MODULE', 'test_django.settings')
os.environ.setdefault("DJANGO_SETTINGS_MODULE", "test_django.settings")
Copy link

Choose a reason for hiding this comment

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

Hardcoding the DJANGO_SETTINGS_MODULE in the WSGI file can lead to configuration errors when deploying to different environments (development, testing, production). It's recommended to manage this setting through environment variables set outside the application codebase, ensuring better security and maintainability.

Suggested Change:
Use environment variables to set DJANGO_SETTINGS_MODULE dynamically based on the deployment environment. This can be achieved by using a tool like dotenv or similar to load environment-specific configurations.


application = get_wsgi_application()
Loading