Fix #1643 : Fully qualified model name + Unique ShortID generation for DTO Model to avoid collision issues.

[v3ss0n]

Pull Request Checklist

• New code has 100% test coverage
• (If applicable) The prose documentation has been updated to reflect the changes introduced by this PR
• (If applicable) The reference documentation has been updated to reflect the changes introduced by this PR

Description

Bug fix for #1643 - where it cause duplicated DTOModel names collision. Attempted to fixed by adding a 12 char unique id to fully qualified name of model.

This PR adds two new test cases to ensure that the generated model names and schema names are unique

• First test case to ensure that the model names generated by the backend are unique to avoid naming collisions between different parts of the code.
• Also next text case check that there are exactly two unique schemas. and that all schema names start with the modle name

Close Issue(s)

Fixes #1643

[peterschutt]

Hey @v3ss0n - thanks for taking this one on!

When we were talking in discord, I was only referring to the msgspec backend type, however there is also the pydantic backend, so perhaps we can create a utility method on AbstractDTOBackend that accepts unique_name as an argument and returns that with the short uid appended to it. We can then call that method from within create_transfer_model_type on either backend implementation.

Also, tests please:)

Thanks again!

[v3ss0n]

Thanks , i am happy to help. i saw other back-end uses it , i was about to ask tha
Edited : added in AbstractDTOBackend , it works.

Should i write test to see if the method is generating unique ids?

[v3ss0n]

@pytest.mark.parametrize("backend_type", [MsgspecDTOBackend, PydanticDTOBackend])
def test_backend_model_name_uniqueness(backend_type: type[AbstractDTOBackend], backend_context: BackendContext) -> None:
    backend = backend_type(backend_context)
    unique_names: set = set()
    fd: FieldDefinitionsType = FieldDefinition(
        name="a",
        default=Empty,
        parsed_type=ParsedType(int),
        default_factory=None,
        dto_field=None,
        unique_model_name="some_module.SomeModel",
    )
    for i in range(100):
        model_class = backend.create_transfer_model_type("some_module.SomeModel", fd)
        model_name = model_class.unique_name
        assert model_name not in unique_names
        unique_names.add(model_name)

I am trying like this , i don't know how to get unique_name back from the transfer_model .

[peterschutt]

@pytest.mark.parametrize("backend_type", [MsgspecDTOBackend, PydanticDTOBackend])
def test_backend_model_name_uniqueness(backend_type: type[AbstractDTOBackend], backend_context: BackendContext) -> None:
    backend = backend_type(backend_context)
    unique_names: set = set()
    fd: FieldDefinitionsType = FieldDefinition(
        name="a",
        default=Empty,
        parsed_type=ParsedType(int),
        default_factory=None,
        dto_field=None,
        unique_model_name="some_module.SomeModel",
    )
    for i in range(100):
        model_class = backend.create_transfer_model_type("some_module.SomeModel", fd)
        model_name = model_class.unique_name
        assert model_name not in unique_names
        unique_names.add(model_name)

I am trying like this , i don't know how to get unique_name back from the transfer_model .

does model_class.__name__ work?

[v3ss0n]

sync with main , resolved conflict at poetry.lock , its ready for review.

[peterschutt]

Almost there, just need to get that test passing.

[peterschutt]

Just tested this on the example app - the first 9 characters of each of those random strings is identical across all of the models which I think is due to the first part of uuid1 being a timestamp. This is why you needed to increase the size of the random string in 8e025cb - because the first chars of a timestamp are too slow-moving to be reliably different.

I think we'd be better of just using something like base64.urlsafe_b64encode(os.urandom(6)) for the random component.

Sorry I didn't pick this up earlier!

[provinzkraut]

I think we'd be better of just using something like base64.urlsafe_b64encode(os.urandom(6))

Why not just use secrets.token_urlsafe(6) for this?

[peterschutt]

I think we'd be better of just using something like base64.urlsafe_b64encode(os.urandom(6))

Why not just use secrets.token_urlsafe(6) for this?

Perfect.

[v3ss0n]

Just tested this on the example app - the first 9 characters of each of those random strings is identical across all of the models which I think is due to the first part of uuid1 being a timestamp. This is why you needed to increase the size of the random string in 8e025cb - because the first chars of a timestamp are too slow-moving to be reliably different.

I think we'd be better of just using something like base64.urlsafe_b64encode(os.urandom(6)) for the random component.

Sorry I didn't pick this up earlier!

I was thinking that too , with uuid1 looks like we need whole of it. it is base off on timestamp so the first chars are not much moving within a few minutes.
I am gonna test with secrets.token_urlsafe(6)

[v3ss0n]

secrets.token_urlsafe() have dashes and undercores , should thate be ok?
results like : dI7YzibZwVA0racC-wh_LPg52Ht3G2qHpooc1QlpTME

[provinzkraut]

secrets.token_urlsafe(6) have dashes and undercores , should thate be ok?

Probably better not to. Try secrets.token_hex. That's just letters and numbers.

[v3ss0n]

secrets.token_urlsafe(6) have dashes and undercores , should thate be ok?

Probably better not to. Try secrets.token_hex. That's just letters and numbers.

thanks , this look better , going to raise to 8 chars to increase randomness.

In [30]: secrets.token_hex(8)
Out[30]: '342e6bed9da5e8a5'

[v3ss0n]

changed to : secrets.token_hex(8) , tested by incresing loop to 100k , passed locally.

[peterschutt]

@all-contributors add @v3ss0n for code!

[allcontributors]

@peterschutt

I've put up a pull request to add @v3ss0n! 🎉

[v3ss0n]

Thanks a lot!!