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

Don't save next autoincrement base if it's going to fail, next #14816

Merged
merged 2 commits into from
Jun 5, 2024

Conversation

uberbrady
Copy link
Collaborator

This one of those "one line PR's with x paragraphs of explanation" -

So the problem we have is that some users want to use autoincrementing asset tags, by default - but also, sometimes, they want to save serial numbers as asset tags too.

Sometimes those serial numbers can be very long strings of digits. And those strings of digits can, sometimes, can arithmetically be larger that PHP_INT_MAX - which also is the same as MySQL's BIGINIT (signed BIGINT to be specific).

This change just makes it so that it will not save the 'next autoincrement tag' if it's going to be greater than, or equal to, PHP_INT_MAX. Just to give that some perspective, that number is: 9,223,372,036,854,775,807.

When people are doing autoincrements, we've typically seen those done as 6 digit numbers, maybe 7. I could even understand up to, maybe, 9 or so. But PHP_INT_MAX is 19 digits wide. You probably don't mean that as your next autoincrement number.

Because this is under the newer, more carefully guarded branch, it will only be invoked when auto_increment is turned on, the prefixes match to what the auto-incrementer is configured for, and everything after the prefix is nothing but pure digits. That seems all pretty safe and reasonable to me.

I did not touch the 'legacy branch' of this code - the else - which tries to guess next autoincrement by doing a simple increment (+1) on whatever the value that was trying to be saved. In trying to keep the code simple, I felt like that might have been an OK compromise to make - but one problem we've run into in the past is when someone does not have autoincrement turned on, but wants to turn it on later - I'm willing to leave this problem here, and make them change the next_autoincrement base to a reasonable number. But we can revisit this decision if this change is not extensive enough.

This does NOT fix people who already have this problem - they will have to change their next_autoincrement value to some kind of 'reasonable value'. But once they've done that, they can use weirdly-long serial numbers as their asset tags and not have to worry about it mucking up their next autoincrement. Or so I think. Please double-check me on this.

Copy link

what-the-diff bot commented Jun 3, 2024

PR Summary

  • Adjustment to Asset Generation Logic in AssetObserver.php
    This change adds an extra layer of validation when creating new assets. Now, the system adds an asset only if its tag number (code that uniquely identifies an asset) follows the conditions. It should be higher than the previously set base tag number and less than the highest number possible in our system. This ensures that our asset generation is in sequence and protects against error cases where incorrect tag numbers might be used.

@snipe
Copy link
Owner

snipe commented Jun 3, 2024

This looks really good - can I get a test or two behind this though?

@snipe snipe merged commit 9fccafa into snipe:master Jun 5, 2024
11 of 12 checks passed
@snipe snipe mentioned this pull request Jun 5, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants