HideyoshiNakazone/jambo
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 12 Wiki Activity

fix(jambo): Fix allOf, anyOf, null and array type parsing #33

Closed
thommann wants to merge 10 commits from main into main
pull from: main
merge into: HideyoshiNakazone:main
Conversation 29 Commits 10 Files Changed 27 +1285 -3766
thommann commented 2025-07-04 09:00:40 +00:00 (Migrated from github.com)
Copy Link

Fix parser, such that:

from pydantic import create_model
from jambo import SchemaConverter

fields = {name: (field.annotation, field) for name, field in TestClass.model_fields.items()}
pydantic_model = create_model(
    TestClass.__name__,
    **fields,
)

schema = TestClass.model_json_schema()
jambo_model = SchemaConverter.build(schema)

gives the same result for null, allOf, anyOf, oneOf, string and array types.

Fix parser, such that: ``` from pydantic import create_model from jambo import SchemaConverter fields = {name: (field.annotation, field) for name, field in TestClass.model_fields.items()} pydantic_model = create_model( TestClass.__name__, **fields, ) schema = TestClass.model_json_schema() jambo_model = SchemaConverter.build(schema) ``` gives the same result for null, allOf, anyOf, oneOf, string and array types.
thommann commented 2025-07-04 11:38:59 +00:00 (Migrated from github.com)
Copy Link

I used the null parser from https://github.com/HideyoshiNakazone/jambo/pull/32

I used the null parser from https://github.com/HideyoshiNakazone/jambo/pull/32
NiveditJain commented 2025-08-07 09:40:43 +00:00 (Migrated from github.com)
Copy Link

Hey, when can we expect this PR to be merged?

Hey, when can we expect this PR to be merged?
HideyoshiNakazone commented 2025-08-19 01:55:55 +00:00 (Migrated from github.com)
Copy Link

Sorry for the wait, i'll take a look into the PR

Sorry for the wait, i'll take a look into the PR
HideyoshiNakazone commented 2025-08-19 02:55:01 +00:00 (Migrated from github.com)
Copy Link

Hey, thanks for working on this PR!

I noticed that it’s quite large, and it would be much easier to review if it were split into smaller, focused PRs.

I also saw that some of the typing_extensions imports were changed to typing. That was intentional in the codebase to maintain compatibility with Python 3.10, so we should avoid changing those for now.

This PR also tries to solve multiple issues at once, but some of those have already been addressed by previous smaller PRs. Because of the size and scope, this one will take significantly longer to review.

Hey, thanks for working on this PR! I noticed that it’s quite large, and it would be much easier to review if it were split into smaller, focused PRs. I also saw that some of the `typing_extensions` imports were changed to `typing`. That was intentional in the codebase to maintain compatibility with Python 3.10, so we should avoid changing those for now. This PR also tries to solve multiple issues at once, but some of those have already been addressed by previous smaller PRs. Because of the size and scope, this one will take significantly longer to review.
HideyoshiNakazone (Migrated from github.com) reviewed 2025-08-19 03:01:41 +00:00
jambo/parser/allof_type_parser.py
HideyoshiNakazone (Migrated from github.com) commented 2025-08-19 03:01:40 +00:00
Copy Link

This change will make that the smallest value between old_value and new_value will became the new max value, this change is faulty.

This change will make that the smallest value between `old_value` and `new_value` will became the new max value, this change is faulty.
HideyoshiNakazone (Migrated from github.com) reviewed 2025-08-19 03:02:05 +00:00
jambo/parser/allof_type_parser.py
HideyoshiNakazone (Migrated from github.com) commented 2025-08-19 03:02:04 +00:00
Copy Link

This change will make that the largest value between old_value and new_value will became the new min value, this change is faulty.

This change will make that the largest value between `old_value` and `new_value` will became the new min value, this change is faulty.
HideyoshiNakazone (Migrated from github.com) reviewed 2025-08-19 03:02:36 +00:00
jambo/parser/allof_type_parser.py
@@ -5,3 +3,3 @@
from typing_extensions import Unpack
from typing import Any, Unpack
HideyoshiNakazone (Migrated from github.com) commented 2025-08-19 03:02:36 +00:00
Copy Link

typing_extensions is prefered for Python3.10 compatibility

`typing_extensions` is prefered for Python3.10 compatibility
HideyoshiNakazone (Migrated from github.com) reviewed 2025-08-19 03:02:51 +00:00
jambo/parser/_type_parser.py
@@ -3,3 +2,3 @@
from pydantic import Field, TypeAdapter
from typing_extensions import Annotated, Any, ClassVar, Generic, Self, TypeVar, Unpack
from typing import Annotated, Any, Generic, Self, TypeVar, Unpack
HideyoshiNakazone (Migrated from github.com) commented 2025-08-19 03:02:51 +00:00
Copy Link

typing_extensions is prefered for Python3.10 compatibility

`typing_extensions` is prefered for Python3.10 compatibility
HideyoshiNakazone (Migrated from github.com) reviewed 2025-08-19 03:03:15 +00:00
jambo/parser/_type_parser.py
@@ -67,3 +57,4 @@
cls, name: str, properties: dict[str, Any], **kwargs: Unpack[TypeParserOptions]
) -> tuple[type, dict]:
"""
Factory method to fetch the appropriate type parser based on properties
HideyoshiNakazone (Migrated from github.com) commented 2025-08-19 03:03:15 +00:00
Copy Link

changed for what reason?

changed for what reason?
HideyoshiNakazone (Migrated from github.com) reviewed 2025-08-19 03:05:28 +00:00
jambo/parser/anyof_type_parser.py
@@ -5,2 +4,3 @@
from pydantic import Field
from typing_extensions import Annotated, Union, Unpack
from typing import Annotated, Unpack
from types import UnionType
HideyoshiNakazone (Migrated from github.com) commented 2025-08-19 03:05:28 +00:00
Copy Link

typing_extensions is prefered for Python3.10 compatibility

`typing_extensions` is prefered for Python3.10 compatibility
HideyoshiNakazone (Migrated from github.com) reviewed 2025-08-19 03:06:55 +00:00
jambo/parser/boolean_type_parser.py
@@ -3,3 +2,3 @@
from jambo.types.type_parser_options import TypeParserOptions
from typing_extensions import Unpack
from typing import Unpack
HideyoshiNakazone (Migrated from github.com) commented 2025-08-19 03:06:54 +00:00
Copy Link

typing_extensions is prefered for Python3.10 compatibility

`typing_extensions` is prefered for Python3.10 compatibility
HideyoshiNakazone (Migrated from github.com) reviewed 2025-08-19 03:08:40 +00:00
jambo/parser/const_type_parser.py
@@ -5,3 +4,3 @@
from pydantic import AfterValidator
from typing_extensions import Annotated, Any, Literal, Unpack
from typing import Annotated, Any, Literal, Unpack
HideyoshiNakazone (Migrated from github.com) commented 2025-08-19 03:08:39 +00:00
Copy Link

typing_extensions is prefered for Python3.10 compatibility

`typing_extensions` is prefered for Python3.10 compatibility
HideyoshiNakazone (Migrated from github.com) reviewed 2025-08-19 03:17:19 +00:00
jambo/parser/const_type_parser.py
HideyoshiNakazone (Migrated from github.com) commented 2025-08-19 03:17:19 +00:00
Copy Link

This is a good idea and implementation, but would have to be merged in a separate PR.

This is a good idea and implementation, but would have to be merged in a separate PR.
HideyoshiNakazone (Migrated from github.com) reviewed 2025-08-19 03:17:39 +00:00
jambo/parser/enum_type_parser.py
@@ -5,2 +3,3 @@
from jambo.types.type_parser_options import TypeParserOptions
from typing_extensions import Unpack
from typing import Unpack
HideyoshiNakazone (Migrated from github.com) commented 2025-08-19 03:17:39 +00:00
Copy Link

typing_extensions is prefered for Python3.10 compatibility

`typing_extensions` is prefered for Python3.10 compatibility
HideyoshiNakazone (Migrated from github.com) reviewed 2025-08-19 03:17:45 +00:00
jambo/parser/float_type_parser.py
@@ -2,3 +2,3 @@
from jambo.types.type_parser_options import TypeParserOptions
from typing_extensions import Unpack
from typing import Unpack
HideyoshiNakazone (Migrated from github.com) commented 2025-08-19 03:17:45 +00:00
Copy Link

typing_extensions is prefered for Python3.10 compatibility

`typing_extensions` is prefered for Python3.10 compatibility
HideyoshiNakazone (Migrated from github.com) reviewed 2025-08-19 03:17:51 +00:00
jambo/parser/int_type_parser.py
@@ -2,3 +2,3 @@
from jambo.types.type_parser_options import TypeParserOptions
from typing_extensions import Unpack
from typing import Unpack
HideyoshiNakazone (Migrated from github.com) commented 2025-08-19 03:17:51 +00:00
Copy Link

typing_extensions is prefered for Python3.10 compatibility

`typing_extensions` is prefered for Python3.10 compatibility
HideyoshiNakazone (Migrated from github.com) reviewed 2025-08-19 03:21:47 +00:00
jambo/parser/_type_parser.py
HideyoshiNakazone (Migrated from github.com) commented 2025-08-19 03:21:47 +00:00
Copy Link

This function shouldn't be a staticmethod, it's to simple and does to little. Make this simple check inside the function body since it is used only two times, or transform this into a utility function in a utils folder.

This function shouldn't be a staticmethod, it's to simple and does to little. Make this simple check inside the function body since it is used only two times, or transform this into a utility function in a utils folder.
HideyoshiNakazone (Migrated from github.com) reviewed 2025-08-19 03:22:10 +00:00
jambo/parser/oneof_type_parser.py
@@ -9,2 +6,2 @@
Annotation = Annotated[Any, ...]
from pydantic import Field, BeforeValidator, TypeAdapter, ValidationError
from typing import Annotated, Unpack, Any
HideyoshiNakazone (Migrated from github.com) commented 2025-08-19 03:22:09 +00:00
Copy Link

typing_extensions is prefered for Python3.10 compatibility

`typing_extensions` is prefered for Python3.10 compatibility
HideyoshiNakazone (Migrated from github.com) reviewed 2025-08-19 03:22:42 +00:00
jambo/parser/oneof_type_parser.py
HideyoshiNakazone (Migrated from github.com) commented 2025-08-19 03:22:42 +00:00
Copy Link

This entire feature should be in a separate PR

This entire feature should be in a separate PR
HideyoshiNakazone (Migrated from github.com) reviewed 2025-08-19 03:22:50 +00:00
jambo/parser/object_type_parser.py
@@ -8,3 +5,1 @@
from typing_extensions import Unpack
import warnings
from typing import Any, Unpack
HideyoshiNakazone (Migrated from github.com) commented 2025-08-19 03:22:50 +00:00
Copy Link

typing_extensions is prefered for Python3.10 compatibility

`typing_extensions` is prefered for Python3.10 compatibility
HideyoshiNakazone (Migrated from github.com) reviewed 2025-08-19 03:23:21 +00:00
jambo/parser/null_type_parser.py
HideyoshiNakazone (Migrated from github.com) commented 2025-08-19 03:23:21 +00:00
Copy Link

Already implemented in a previous PR that was merged, sorry

Already implemented in a previous PR that was merged, sorry
HideyoshiNakazone (Migrated from github.com) reviewed 2025-08-19 03:23:34 +00:00
jambo/parser/ref_type_parser.py
@@ -5,3 +2,3 @@
from jambo.types.type_parser_options import TypeParserOptions
from typing_extensions import ForwardRef, Literal, Union, Unpack
from typing import Any, ForwardRef, Literal, TypeVar, Unpack
HideyoshiNakazone (Migrated from github.com) commented 2025-08-19 03:23:34 +00:00
Copy Link

typing_extensions is prefered for Python3.10 compatibility

`typing_extensions` is prefered for Python3.10 compatibility
HideyoshiNakazone (Migrated from github.com) reviewed 2025-08-19 03:23:41 +00:00
jambo/parser/string_type_parser.py
@@ -5,2 +4,2 @@
from pydantic import AnyUrl, EmailStr, TypeAdapter, ValidationError
from typing_extensions import Unpack
from pydantic import EmailStr, HttpUrl, IPvAnyAddress, FilePath
from typing import Unpack
HideyoshiNakazone (Migrated from github.com) commented 2025-08-19 03:23:41 +00:00
Copy Link

typing_extensions is prefered for Python3.10 compatibility

`typing_extensions` is prefered for Python3.10 compatibility
HideyoshiNakazone (Migrated from github.com) reviewed 2025-08-19 03:24:56 +00:00
jambo/types/json_schema_type.py
HideyoshiNakazone (Migrated from github.com) commented 2025-08-19 03:24:56 +00:00
Copy Link

A large change with little impact, is there a necessity for this change?

A large change with little impact, is there a necessity for this change?
HideyoshiNakazone (Migrated from github.com) reviewed 2025-08-19 03:25:23 +00:00
jambo/types/type_parser_options.py
@@ -4,3 +3,1 @@
RefCacheDict = MutableMapping[str, ForwardRef | type | None]
from typing import TypedDict
HideyoshiNakazone (Migrated from github.com) commented 2025-08-19 03:25:23 +00:00
Copy Link

typing_extensions is prefered for Python3.10 compatibility

`typing_extensions` is prefered for Python3.10 compatibility
HideyoshiNakazone (Migrated from github.com) reviewed 2025-08-19 03:28:23 +00:00
jambo/parser/allof_type_parser.py
HideyoshiNakazone (Migrated from github.com) commented 2025-08-19 03:28:23 +00:00
Copy Link

Basically, for the AllOf be valid for both values the largest interval must be used, otherwise the AllOf requirement will break.

Basically, for the AllOf be valid for both values the largest interval must be used, otherwise the AllOf requirement will break.
HideyoshiNakazone (Migrated from github.com) reviewed 2025-08-19 03:28:28 +00:00
jambo/parser/allof_type_parser.py
HideyoshiNakazone (Migrated from github.com) commented 2025-08-19 03:28:28 +00:00
Copy Link

Basically, for the AllOf be valid for both values the largest interval must be used, otherwise the AllOf requirement will break

Basically, for the AllOf be valid for both values the largest interval must be used, otherwise the AllOf requirement will break
HideyoshiNakazone commented 2025-08-19 03:39:51 +00:00 (Migrated from github.com)
Copy Link

After reviewing this PR, I’ve decided not to merge it as it currently stands.

The improvements in ConstParser and the OneOf implementation are valuable, but they should each be proposed in separate, focused PRs.

For this branch in its current form, neither change will be merged.

Thanks @thommann for the PR! If you’d still like to contribute, I’ll be looking forward to PRs focused on ConstParser and on the OneOf implementation.

After reviewing this PR, I’ve decided not to merge it as it currently stands. The improvements in `ConstParser` and the `OneOf` implementation are valuable, but they should each be proposed in separate, focused PRs. For this branch in its current form, neither change will be merged. Thanks @thommann for the PR! If you’d still like to contribute, I’ll be looking forward to PRs focused on `ConstParser` and on the `OneOf` implementation.
HideyoshiNakazone commented 2025-08-19 03:53:15 +00:00 (Migrated from github.com)
Copy Link

Just to address this initial issue @thommann :

From the current state of the main branch

from pydantic import create_model, BaseModel


class TestClass(BaseModel):
    a_thing: str | None

pydantics_model_dump = TestClass.model_json_schema()

jambo_model = SchemaConverter.build(
    pydantics_model_dump
)

jambo_model_dump = jambo_model.model_json_schema()

print(pydantics_model_dump)
# {'properties': {'a_thing': {'anyOf': [{'type': 'string'}, {'type': 'null'}], 'title': 'A Thing'}}, 'required': ['a_thing'], 'title': 'TestClass', 'type': 'object'}

print(jambo_model_dump)
# {'properties': {'a_thing': {'anyOf': [{'type': 'string'}, {'default': None, 'type': 'null'}], 'title': 'A Thing'}}, 'required': ['a_thing'], 'title': 'TestClass', 'type': 'object'}

As you can see, the generated JSON schema is not significantly different.
I don’t think this is an issue as-is.

If you can share the specific class that caused problems in your case, we can try to reproduce it and find a solution tailored to your test case.

Fix parser, such that:

from pydantic import create_model
from jambo import SchemaConverter

fields = {name: (field.annotation, field) for name, field in TestClass.model_fields.items()}
pydantic_model = create_model(
    TestClass.__name__,
    **fields,
)

schema = TestClass.model_json_schema()
jambo_model = SchemaConverter.build(schema)

gives the same result for null, allOf, anyOf, oneOf, string and array types.

Just to address this initial issue @thommann : From the current state of the main branch ```python from pydantic import create_model, BaseModel class TestClass(BaseModel): a_thing: str | None pydantics_model_dump = TestClass.model_json_schema() jambo_model = SchemaConverter.build( pydantics_model_dump ) jambo_model_dump = jambo_model.model_json_schema() print(pydantics_model_dump) # {'properties': {'a_thing': {'anyOf': [{'type': 'string'}, {'type': 'null'}], 'title': 'A Thing'}}, 'required': ['a_thing'], 'title': 'TestClass', 'type': 'object'} print(jambo_model_dump) # {'properties': {'a_thing': {'anyOf': [{'type': 'string'}, {'default': None, 'type': 'null'}], 'title': 'A Thing'}}, 'required': ['a_thing'], 'title': 'TestClass', 'type': 'object'} ``` As you can see, the generated JSON schema is not significantly different. I don’t think this is an issue as-is. If you can share the specific class that caused problems in your case, we can try to reproduce it and find a solution tailored to your test case. > Fix parser, such that: > > ``` > from pydantic import create_model > from jambo import SchemaConverter > > fields = {name: (field.annotation, field) for name, field in TestClass.model_fields.items()} > pydantic_model = create_model( > TestClass.__name__, > **fields, > ) > > schema = TestClass.model_json_schema() > jambo_model = SchemaConverter.build(schema) > ``` > > gives the same result for null, allOf, anyOf, oneOf, string and array types.

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
HideyoshiNakazone
Labels
Clear labels
bug
Something isn't working
 documentation
Improvements or additions to documentation
 duplicate
This issue or pull request already exists
 enhancement
New feature or request
 good first issue
Good for newcomers
 help wanted
Extra attention is needed
 invalid
This doesn't seem right
 question
Further information is requested
 wontfix
This will not be worked on
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
HideyoshiNakazone
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: HideyoshiNakazone/jambo#33
Delete Branch "main"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?