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

warehouse: PEP 740 models #16625

Merged
merged 15 commits into from
Sep 11, 2024
Merged

warehouse: PEP 740 models #16625

merged 15 commits into from
Sep 11, 2024

Conversation

woodruffw
Copy link
Member

This is a breakout from #16624,
to reduce the complexity/headache of zippered reverts.

Signed-off-by: William Woodruff william@trailofbits.com

This is a breakout from pypi#16624,
to reduce the complexity/headache of zippered reverts.

Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw woodruffw mentioned this pull request Sep 3, 2024
11 tasks
@woodruffw woodruffw self-assigned this Sep 3, 2024
Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw woodruffw marked this pull request as ready for review September 3, 2024 20:06
@woodruffw woodruffw requested a review from a team as a code owner September 3, 2024 20:06
woodruffw and others added 7 commits September 3, 2024 19:46
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
@DarkaMaul
Copy link
Contributor

I added a simple test and Factory in 34ec661 that can easily be reverted if needed.

However, just thinking out loud here, I think we should also have a version column in the Provenance that contains the Provenance object version.

The current design forces us to keep a forward-compatible version of the Provenance object to use the opaque JSON object with a model_validate_json call.
Having a separate field allows us to deviate from the original design and handle the deserialization logic without resorting to try/catch.

@woodruffw
Copy link
Member Author

However, just thinking out loud here, I think we should also have a version column in the Provenance that contains the Provenance object version.

The current design forces us to keep a forward-compatible version of the Provenance object to use the opaque JSON object with a model_validate_json call.

I might be under-thinking it, but I don't think this will be an issue -- if we need to roll the provenance version forwards, we can always do something like TypeAdapter(Provenance | ProvenanceV2).validate_json(...) to avoid needing to explicitly test each version.

(But also, if we need to bump the version, we can always add a version column with default=1. So I don't think we're excessively constraining ourselves at the moment.)

Comment on lines 27 to 30
provenance = {}
provenance_digest = factory.LazyAttribute(
lambda o: hashlib.sha256(o.file.filename.encode("utf8")).hexdigest()
)
Copy link
Member Author

Choose a reason for hiding this comment

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

Modeling these fields accurately requires access to the services that we're defining in #16624, so I'm inclined to punt on adding the factory in this PR 🙂

I'm going to save a patch for this and apply it on top of #16624, however.

@di di merged commit cc0b96d into pypi:main Sep 11, 2024
18 checks passed
@di di deleted the ww/pep740-models branch September 11, 2024 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants