-
Notifications
You must be signed in to change notification settings - Fork 276
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
Require id property #476
Require id property #476
Conversation
…x some specs (added a call to id_property_info at the top of #create_model to make sure the UUID is there. There is probably a better way)
…of definition (using "defined" gem). Some more cleanup and spec fixes
Conflicts: spec/e2e/active_model_spec.rb
…nt outside of the transaction where data is change
When I say it works, I should clarify that the specs pass. Going to try it in an actual app now ;) |
Note: first/last/etc... should use uuid, not ID() |
Really nice work ! |
Couldn't Sorry if that's already been discussed, I'm coming into this kind of late. :-) |
@subvertallchris I think tried that, but please let me know if I missed something ;) The problem is that you can call |
@andreasronge Yup, I agree. If you come up with a way to not do that, I would be so happy ;) |
Ahh, now I see! I did see a few of your comments where you mentioned difficulty with the constraint but it was just bits and pieces so I couldn't quite place it. I guess it works fine for my gem since the whole idea is that it's there for global use and the user won't be declaring it twice. |
Is declaring it a second time really a problem if they aren't trying to define a constraint on the same property? That is to say, if they are defining a different id_property, that's just an additional unique constraint, and what harm is there in that? The first one is still there but it doesn't matter unless they try to reuse that property name as a vanilla non-unique property, which we can prevent by adding any already-declared id properties to the list of restricted property names. (The word "property" appears too many times in this comment.) |
A good question. Maybe not if unique constraints don't constrain null/non-existant values, but it still seems somewhat messy polluting users' databases with unnecessary constraints. Another thing that I thought of that I didn't really explore is having some call which is required to start using the models. So the models are all defined and then you call something like |
In that case, what about using Cypher to drop the existing constraint in the event the user declares id property on the model? |
Huh, you know, that's not a bad idea ;) I just did that in the specs to make sure the test database is cleared out when re-used ;) I'm off to sleep now and I'm going to be out exploring Paris tomorrow (sans ordinateur portable), but I'll think about that and likely implement that. Also for @andreasronge I agree about the rebase/squash and will do that when I'm done |
@subvertallchris sounds like a good solution. I guess setting the index constraints twice only occurs if there is already a session available since it will otherwise be delayed until session is available, see |
Two thoughts:
|
Started over with PR #478 |
It friggin' works!
Require that
id_property
is called for all models. Default to former behavior ofauto: :uuid
whenid_property :field
is called