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

Allow for non unique state IDs #144

Merged
merged 14 commits into from
Jul 23, 2024
Merged

Conversation

inxilpro
Copy link
Contributor

@inxilpro inxilpro commented Jun 7, 2024

Right now, Verbs snapshots have a unique id primary key, which means that you can only have one of any given ID snapshotted at any given time. This is fine if you use snowflakes for everything, but if you don't that can be an issue.

@skylerkatz and I paired on this for a while, and I know there were issues remaining, but I can't remember what they are. I'm going to post this up as a draft while I review it and maybe add some more tests.

@inxilpro inxilpro marked this pull request as ready for review July 17, 2024 19:30
Copy link
Collaborator

@skylerkatz skylerkatz left a comment

Choose a reason for hiding this comment

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

Overall this makes sense to me, and it does resolve our issue with using legacy model records as state ids.

I still don't love the explicit reliance on Snowflakes even if you have configured some other key generation. But, that is a discussion for a different day.

Comment on lines +51 to +54
if ($ephemeral === $not_found) {
$this->setEphemeral($target, $key, $default);
$ephemeral = $default;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not the standard behavior of the $default value in one of these getters, but is convenient here. I could see an argument for making this more explicit in the calling code, but it would add a number of new lines of boilerplate…

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is fine

@inxilpro
Copy link
Contributor Author

@skylerkatz I forgot to push up a couple changes. Can you take another look?

Also—I agree that we should think about allowing Verbs to run without snowflakes at all. Let's discuss at some point.

Copy link
Contributor

@DanielCoulbourne DanielCoulbourne left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@skylerkatz skylerkatz left a comment

Choose a reason for hiding this comment

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

New changes LGTM

@DanielCoulbourne DanielCoulbourne merged commit c1eb12a into main Jul 23, 2024
@DanielCoulbourne DanielCoulbourne deleted the allow-for-non-unique-state-ids branch July 23, 2024 22:17
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