Support Redocly's x-enumDescriptions OpenAPI extension with correct mapping format
#125
Conversation
|
Merging to
|
Welcome @Copilot! 🎉Great PR! I've analyzed your code changes for:
Ready to see the full review?
Let's make your code even better together! 🚀 |
|
Bito Automatic Review Skipped - Draft PR |
|
Here's the code health analysis summary for commits Analysis Summary
|
Co-authored-by: jhassine <1729905+jhassine@users.noreply.github.com>
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Join our Discord community for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
test_enum_descriptions.py
Outdated
| """Test script to understand current enum handling and test x-enumDescriptions.""" | ||
|
|
||
| import os | ||
| os.environ['DJANGO_SETTINGS_MODULE'] = 'tests.settings' |
There was a problem hiding this comment.
Setting the Django settings module directly in the script (os.environ['DJANGO_SETTINGS_MODULE'] = 'tests.settings') is not flexible and can lead to issues when the script is run in different environments. Recommendation: Consider using environment variables or configuration files to set the Django settings module. This approach would allow the script to adapt more easily to different environments without modifying the code.
test_enum_descriptions.py
Outdated
| print("Current OpenAPI schema:") | ||
| import json | ||
| print(json.dumps(schema, indent=2)) No newline at end of file |
There was a problem hiding this comment.
The use of print statements for outputting the OpenAPI schema is not ideal for production or automated testing environments as it does not allow for easy redirection or capture of the output. Recommendation: Replace print statements with a logging framework or write the output to a file. This would provide better control over output handling and facilitate integration with other systems or automated tests.
| import os | ||
| os.environ['DJANGO_SETTINGS_MODULE'] = 'tests.settings' | ||
| import django | ||
| django.setup() |
There was a problem hiding this comment.
The script sets the Django environment variables and initializes Django directly within the code. This approach is not flexible and can lead to issues when the script is run in different environments or configurations.
Recommendation: Consider managing environment settings outside of the script, for example, by using environment variables set through a deployment configuration or a .env file. This would make the script more portable and easier to configure across different environments.
test_schema_extra.py
Outdated
| from enum import Enum | ||
| from typing import Annotated | ||
| from pydantic import BaseModel, Field | ||
| from pydantic._internal._generate_schema import GetJsonSchemaFunction |
There was a problem hiding this comment.
The script imports GetJsonSchemaFunction from pydantic._internal._generate_schema, which is an internal part of Pydantic. Using internal APIs is risky as they can change without warning in new releases of the library, potentially breaking your code.
Recommendation: Avoid using internal APIs if possible. Check if Pydantic provides a public API that offers the functionality you need. If not, consider reaching out to the Pydantic maintainers to discuss making this functionality part of the public API.
|
Diff Coverage: Not applicable. There was no coverage data reported for the files in this diff. Total Coverage: This PR will increase coverage by 0.52%. File Coverage Changes
🛟 Help
|
| django.setup() | ||
|
|
||
| from django.db import models | ||
| from django.utils.translation import gettext_lazy as _ | ||
| from tests.utils import get_openapi_schema_from_field | ||
|
|
||
|
|
||
| class YearInSchool(models.TextChoices): | ||
| FRESHMAN = "FR", _("Freshman") | ||
| SOPHOMORE = "SO", _("Sophomore") | ||
| JUNIOR = "JR", _("Junior") | ||
| SENIOR = "SR", _("Senior") | ||
| GRADUATE = "GR", _("Graduate") | ||
|
|
||
|
|
||
| # Test current behavior | ||
| field = models.CharField(max_length=2, choices=YearInSchool.choices) | ||
| schema = get_openapi_schema_from_field(field) | ||
|
|
||
| print("Current OpenAPI schema:") | ||
| import json | ||
|
|
||
| print(json.dumps(schema, indent=2)) |
There was a problem hiding this comment.
The script lacks error handling, which could lead to unclear failures if any part of the Django setup or schema generation fails. Recommendation: Implement try-except blocks around critical operations like django.setup() and get_openapi_schema_from_field(field). Log the errors or handle them appropriately to provide feedback on what went wrong.
| # Test current behavior | ||
| field = models.CharField(max_length=2, choices=YearInSchool.choices) | ||
| schema = get_openapi_schema_from_field(field) | ||
|
|
||
| print("Current OpenAPI schema:") | ||
| import json | ||
|
|
||
| print(json.dumps(schema, indent=2)) |
There was a problem hiding this comment.
The script is used for testing but does not utilize a formal testing framework or assertions to verify the correctness of the output against expected results. Recommendation: Integrate a testing framework such as unittest or pytest and use assertions to validate the OpenAPI schema. This will make the script more robust and useful as a test tool, allowing systematic handling of test failures and successes.
| schema = TestModel.model_json_schema() | ||
| print("Test schema with custom __get_pydantic_json_schema__:") | ||
| print(json.dumps(schema, indent=2)) |
There was a problem hiding this comment.
The script lacks error handling around critical operations such as generating the JSON schema and printing it. This could lead to unhandled exceptions if there are issues during these operations.
Recommendation: Implement try-except blocks around the schema generation and JSON serialization to handle potential exceptions gracefully. Provide meaningful error messages to aid in debugging if an error occurs.
test_schema_extra.py
Outdated
| ) -> JsonSchemaValue: | ||
| """Add custom x-enumDescriptions to the enum schema.""" | ||
| json_schema = handler(core_schema) | ||
| json_schema["x-enumDescriptions"] = ["Choice A", "Choice B", "Choice C"] |
There was a problem hiding this comment.
The descriptions for the enum values in TestEnum are hardcoded within the __get_pydantic_json_schema__ method. This approach is not flexible and makes the code less maintainable if the enum values or their descriptions change.
Recommendation: Consider associating descriptions directly with enum members or using a dictionary to map enum values to their descriptions. This would make the code more maintainable and flexible.
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #125 +/- ##
==========================================
+ Coverage 87.48% 87.95% +0.46%
==========================================
Files 42 43 +1
Lines 1998 2017 +19
Branches 121 125 +4
==========================================
+ Hits 1748 1774 +26
+ Misses 250 243 -7 |
❌ 23 blocking issues (23 total)
|
![qltysh[bot]](https://avatars.githubusercontent.com/in/890766?s=60&v=4){kind=small}
| @@ -0,0 +1,31 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
|
|
||
| from django.db import models | ||
| from django.utils.translation import gettext_lazy as _ | ||
| from tests.utils import get_openapi_schema_from_field |
|
|
||
|
|
||
| # Create a test enum with custom schema modifier | ||
| class TestEnum(str, Enum): |
|
|
||
|
|
||
| schema = TestModel.model_json_schema() | ||
| print("Test schema with custom __get_pydantic_json_schema__:") |
|
|
||
| schema = TestModel.model_json_schema() | ||
| print("Test schema with custom __get_pydantic_json_schema__:") | ||
| print(json.dumps(schema, indent=2)) |
| from tests.utils import get_openapi_schema_from_field | ||
|
|
||
|
|
||
| class YearInSchool(models.TextChoices): |
There was a problem hiding this comment.
Missing docstring in public class [ruff:D101]
| class YearInSchool(models.TextChoices): |
Added a docstring to the
YearInSchoolclass to comply with the D101 rule, which requires a docstring for public classes. The docstring provides a brief description of the class's purpose.
Caution: AI-generated change
| from enum import Enum | ||
| from typing import Annotated | ||
| from pydantic import BaseModel, Field |
There was a problem hiding this comment.
Found 2 issues:
1. Module level import not at top of file [ruff:E402]
2. pydantic.Field imported but unused [ruff:F401]
| from enum import Enum | |
| from typing import Annotated | |
| from pydantic import BaseModel, Field | |
| from typing import Annotated | |
| from enum import Enum |
The import statement for the 'enum' module was moved to the top of the file to comply with the E402 rule, which requires all module-level imports to be at the top of the file.
Caution: AI-generated change
|
Bito Automatic Review Skipped - Draft PR |
There was a problem hiding this comment.
@copilot did you check the the linked specs as you did not seem to follow the correct structure as indicated in this example: https://redocly.com/docs/realm/content/api-docs/openapi-extensions/x-enum-descriptions#examples
The following example shows a schema with two short-named options, and the x-enumDescriptions entry to list all enum entries and give additional context for each:
components:
schemas:
TicketType:
description: Type of ticket being purchased. Use `general` for regular museum entry and `event` for tickets to special events.
type: string
enum:
- event
- general
x-enumDescriptions:
event: Event Tickets _(timed entry)_
general: General Admission
example: eventCo-authored-by: jhassine <1729905+jhassine@users.noreply.github.com>
| os.environ["DJANGO_SETTINGS_MODULE"] = "tests.settings" | ||
| import django | ||
|
|
||
| django.setup() |
There was a problem hiding this comment.
The script sets the Django environment variables and initializes Django directly within the code. This approach is not flexible and can lead to issues when the script is run in different environments or configurations.
Recommendation: Consider managing environment settings outside of the script, for example, by using environment variables set through a deployment configuration or a .env file. This would make the script more portable and easier to configure across different environments.
| schema = TestModel.model_json_schema() | ||
| print("Test schema with custom __get_pydantic_json_schema__:") | ||
| print(json.dumps(schema, indent=2)) |
There was a problem hiding this comment.
The script lacks error handling around critical operations such as generating the JSON schema and printing it. This could lead to unhandled exceptions if there are issues during these operations.
Recommendation: Implement try-except blocks around the schema generation and JSON serialization to handle potential exceptions gracefully. Provide meaningful error messages to aid in debugging if an error occurs.
| @classmethod | ||
| def __get_pydantic_json_schema__( | ||
| cls, core_schema: core_schema.CoreSchema, handler: GenerateJsonSchema | ||
| ) -> JsonSchemaValue: | ||
| """Add custom x-enumDescriptions to the enum schema.""" | ||
| json_schema = handler(core_schema) | ||
| json_schema["x-enumDescriptions"] = { | ||
| "a": "Choice A", | ||
| "b": "Choice B", | ||
| "c": "Choice C", | ||
| } | ||
| return json_schema |
There was a problem hiding this comment.
Potential Data Race and Modularity Concerns
The method __get_pydantic_json_schema__ modifies the json_schema dictionary directly. This approach can lead to data races if multiple threads are attempting to modify the schema simultaneously. Additionally, the method is modifying the schema in a way that is tightly coupled to the specific enum class.
Recommendation:
- To avoid data races, consider using thread-safe mechanisms or deep copying the schema before modification.
- Increase modularity by separating the concerns: create a dedicated function or class responsible for schema modifications, which can be reused across different parts of the application or for different enums.
| os.environ["DJANGO_SETTINGS_MODULE"] = "tests.settings" | ||
| import django | ||
|
|
There was a problem hiding this comment.
Found 2 issues:
1. Module level import not at top of file [ruff:E402]
2. Import block is un-sorted or un-formatted [ruff:I001]
| os.environ["DJANGO_SETTINGS_MODULE"] = "tests.settings" | |
| import django | |
| from django.db import models | |
| from django.utils.translation import gettext_lazy as _ | |
| from tests.utils import get_openapi_schema_from_field |
The imports from Django and the test utilities were moved to the top of the file to comply with the E402 rule, which requires module-level imports to be at the top of the file. This change ensures that all imports are declared before any other code execution, such as setting environment variables or calling setup functions.
Caution: AI-generated change
| from django.db import models | ||
| from django.utils.translation import gettext_lazy as _ |
There was a problem hiding this comment.
Module level import not at top of file [ruff:E402]
| from django.db import models | |
| from django.utils.translation import gettext_lazy as _ | |
| from django.utils.translation import gettext_lazy as _ | |
| from django.db import models |
The import statement for
gettext_lazywas moved to the top of the file to comply with PEP 8 guidelines, which require all import statements to be at the top of the file. This resolves the E402 error reported by theruffstatic analysis tool.
Caution: AI-generated change
| os.environ["DJANGO_SETTINGS_MODULE"] = "tests.settings" | ||
| import django | ||
|
|
||
| django.setup() | ||
|
|
||
| from enum import Enum | ||
| from typing import Annotated |
There was a problem hiding this comment.
Found 2 issues:
1. Module level import not at top of file [ruff:E402]
2. Import block is un-sorted or un-formatted [ruff:I001]
| os.environ["DJANGO_SETTINGS_MODULE"] = "tests.settings" | |
| import django | |
| django.setup() | |
| from enum import Enum | |
| from typing import Annotated | |
| from enum import Enum | |
| os.environ["DJANGO_SETTINGS_MODULE"] = "tests.settings" | |
| import django | |
| django.setup() |
The import statement for the
Enummodule was moved to the top of the file to comply with the E402 rule, which requires all module-level imports to be at the top of the file.
Caution: AI-generated change
| from typing import Annotated | ||
| from pydantic import BaseModel, Field |
There was a problem hiding this comment.
Found 2 issues:
1. Module level import not at top of file [ruff:E402]
2. typing.Annotated imported but unused [ruff:F401]
| from typing import Annotated | |
| from pydantic import BaseModel, Field | |
| from pydantic import BaseModel, Field | |
| from typing import Annotated |
The import statement for
Annotatedfrom thetypingmodule was moved to the top of the file to comply with the E402 rule, which requires all module-level imports to be at the top of the file.
Caution: AI-generated change
| from enum import Enum | ||
| from typing import Annotated | ||
| from pydantic import BaseModel, Field | ||
| from pydantic.json_schema import JsonSchemaValue, GenerateJsonSchema |
There was a problem hiding this comment.
Module level import not at top of file [ruff:E402]
| from enum import Enum | |
| from typing import Annotated | |
| from pydantic import BaseModel, Field | |
| from pydantic.json_schema import JsonSchemaValue, GenerateJsonSchema | |
| from pydantic.json_schema import JsonSchemaValue, GenerateJsonSchema | |
| from enum import Enum | |
| from typing import Annotated | |
| from pydantic import BaseModel, Field |
The import statement for
JsonSchemaValueandGenerateJsonSchemafrompydantic.json_schemawas moved to the top of the file to comply with the E402 rule, which requires all module-level imports to be at the top of the file.
Caution: AI-generated change
| from typing import Annotated | ||
| from pydantic import BaseModel, Field | ||
| from pydantic.json_schema import JsonSchemaValue, GenerateJsonSchema | ||
| from pydantic_core import core_schema |
| from pydantic import BaseModel, Field | ||
| from pydantic.json_schema import JsonSchemaValue, GenerateJsonSchema | ||
| from pydantic_core import core_schema | ||
| import json |
|
|
||
|
|
||
| # Test how to add custom schema properties | ||
| class TestModel(BaseModel): |
You're absolutely right! I've now implemented the correct format per the Redocly specification. The Before (incorrect): "x-enumDescriptions": ["Freshman", "Sophomore", "Junior", "Senior", "Graduate"]After (correct per Redocly spec): "x-enumDescriptions": {
"FR": "Freshman",
"SO": "Sophomore",
"JR": "Junior",
"SR": "Senior",
"GR": "Graduate"
}The implementation now properly follows the Redocly example you provided and works with all Django choice types (TextChoices, IntegerChoices, and tuple choices). Fixed in commit b4e039e. |
x-enumDescriptions OpenAPI extensionx-enumDescriptions OpenAPI extension with correct mapping format
|
Bito Automatic Review Skipped - Draft PR |
Implements support for Redocly's
x-enumDescriptionsOpenAPI extension to include Django choice labels as descriptions in the generated OpenAPI schema, using the correct mapping format per the specification.Problem
Django models with choices (TextChoices, IntegerChoices, or tuple choices) were generating OpenAPI schemas without descriptive information about what each enum value represents. This made the generated API documentation less user-friendly.
Solution
Added
x-enumDescriptionsextension to enum fields in OpenAPI schemas with the correct format:{ "properties": { "year_in_school": { "enum": ["FR", "SO", "JR", "SR", "GR"], "type": "string", "x-enumDescriptions": { "FR": "Freshman", "SO": "Sophomore", "JR": "Junior", "SR": "Senior", "GR": "Graduate" } } } }Implementation
EnumWithDescriptionsandIntEnumWithDescriptionsclasses that extend Python's Enum/IntEnum with__get_pydantic_json_schema__methodDjangoFieldHandler.get_pydantic_type()to use these custom enum classes and extract choice labelsFeatures
gettext_lazytranslated labelsFixes #124
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.
This change is