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

Use existing nonzero values in autoincr fields instead of overwriting with new autoincr value #29

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Use existing nonzero values in autoincr fields instead of overwriting with new autoincr value #29

wants to merge 1 commit into from

Conversation

sqs
Copy link
Contributor

@sqs sqs commented Nov 14, 2014

Previously, if a field mapped to an auto-incremented column, its value
was thrown away during inserts and was replaced by the
auto-incremented value reported by the database. This commit changes
that behavior. Now, if the field value is nonzero, the existing value
is inserted into the database and the auto-incrementing sequence is
not used. If the field value is 0, the previous behavior still
applies.

This breaks code that expects modl to overwrite nonzero autoincr
fields with the next value from the auto-incrementing sequence.

Motivation: When writing tests for code that uses modl, it's nice to be able to insert rows with a known ID, as opposed to having to insert it and then read it back to see the ID. Also, if you need to insert something with a known ID in your application, you need to use raw SQL (or create a new DbMap without the key's autoincr prop set) because there is currently no way to do so in modl.

All tests pass for me (PostgreSQL, MySQL, SQLite3) ✅

Issues/questions for @jmoiron:

  • Is query plan caching thread-safe? I don't see any explicit locks. I can see how the choice of types and values makes it possibly thread-safe, but that's based on my fuzzy mental model of Go internal data structure semantics.
  • As the commit message and this PR notes, this could break existing code.

… with new autoincr value

Previously, if a field mapped to an auto-incremented column, its value
was thrown away during inserts and was replaced by the
auto-incremented value reported by the database. This commit changes
that behavior. Now, if the field value is nonzero, the existing value
is inserted into the database and the auto-incrementing sequence is
not used. If the field value is 0, the previous behavior still
applies.

This breaks code that expects modl to overwrite nonzero autoincr
fields with the next value from the auto-incrementing sequence.
@jmoiron
Copy link
Owner

jmoiron commented Nov 17, 2014

I'm going to look into this tomorrow, specifically answering question 1 and figuring out the ways it might break existing code so I can document it. Superficially, it seems like the behavioral change of just using the supplied ID is the only thing, but I'll look into it a bit deeper. Great change though, I totally agree with this behavior.

@sqs
Copy link
Contributor Author

sqs commented Nov 30, 2014

Just a friendly follow-up to see if you've had a chance to look into those things.

@jmoiron
Copy link
Owner

jmoiron commented Dec 2, 2014

Sorry for the delay @sqs! I'm going on holiday in a few days which actually means I'll have plenty of downtime to really go through this and make sure it's all right.

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.

None yet

2 participants