Add published field to Release #17257
Conversation
|
Should this follow the same pattern we have for yanking, where there are yanking events with timestamps, and we have a |
I like the idea as it would also not modify how new Releases are created. |
f4161f6 to
60de6e8
Compare
There was a problem hiding this comment.
LGTM, nice work @DarkaMaul!
Given that published is currently invariant as True, this shouldn't cause any observable index changes. However, for the follow-up where we make it controllable, I think we should add some integration tests to make sure the index responses are what we expect.
warehouse/legacy/api/json.py
Outdated
| .filter( | ||
| Release.project == project, | ||
| Release.published.is_(True), | ||
| ) |
There was a problem hiding this comment.
I am very wary of this pattern, where the default gives us all releases and we have to manually filter those that are published. I think it's inevitable that we will miss this filter somewhere and this will result in the wrong releases being returned.
I think instead we need something that behaves like https://github.com/flipbit03/sqlalchemy-easy-softdelete, which makes all queries contain this filter by default, and requires manually querying for the unpublished releases instead.
(There is some additional discussion about this in #6091 because we have the same fundamental issue with any mechanics for soft deletes as well.)
There was a problem hiding this comment.
Great call! I've updated the PR to use an SQLAlchemy event listener and intercept SELECT statements to rewrite them with an exclusion clause.
| state.is_select | ||
| and not state.is_column_load | ||
| and not state.is_relationship_load | ||
| and not state.statement.get_execution_options().get("include_staged", False) |
There was a problem hiding this comment.
FLAG: This is added to allow querying for the published=False query but is not used in this PR.
This PR introduces the following changes :
publishedfield to the Release modela migration to update every existing releases and set their published value to be the creation timePart of #17230
Some notes:
I dislike how I changed the defaultReleasecreation call to force the publish time to be set to a timestamp, but I did not find a better solution. This created a new quirk: every time someone wants to create a new Release, they must make sure thepublishedfield is set.The problem boils down to not setting a value forpublishat the release creation, which would either semantically mean the time is not set (the solution I chose) or use a default value (e.g., the current timestamp). When choosing the latter case, we would need a placeholder value on the Python side (e.g.,published=False) and to convert this placeholder to an explicitnullvalue ( using forcing-null-on-a-column-with-a-default )The new implementation uses a boolean field.