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

Set the primary key to a new record #143

Closed
maiha opened this issue Feb 15, 2018 · 8 comments
Closed

Set the primary key to a new record #143

maiha opened this issue Feb 15, 2018 · 8 comments

Comments

@maiha
Copy link
Contributor

maiha commented Feb 15, 2018

We can set the primary key for a new record on Rails(5), but can't on Granite.

  • Rails(5) : can create a new record with id=1
user = User.new(id: 1, name: "test")  # => #<User id: 1, name: "test">
user.save! # => INSERT INTO `users` (`id`, `name`) VALUES (1, 'test')
  • Granite : creates a new record with max id
user = User.new(id: 1, name: "test") # => #<User:0x13faf00 @id=nil, @name="test">
user.save # => INSERT INTO users (name) VALUES (?): ["test"]

Although we can set id by using setter rather than mass assignment, UPDATE is executed even if it is a new record. In this case, save succeeds but no new records will be created.

user = User.new(name: "test")
user.id = 1
user # =>#<User:0x2420f00 @id=1, @name="test">
user.save # => UPDATE users SET name=? WHERE id=?: ["test", 1]
User.count # => 0

I like the behavior of Rails as follows. Also, I would like to modify the code like that.

  • permits the primary key for mass assignment
  • respects whether the object is a new record or not

Thought?

@drujensen
Copy link
Member

drujensen commented Feb 15, 2018

If the database column for the primary key is (BIG)SERIAL, I don't think you should manually insert your own ID into the field. You will get out of wack pretty quickly doing that.

Can you explain your use case? Is your primary key using (BIG)SERIAL? If not, are you planning of generating your own unique id's?

We talked about allowing someone to disable the primary and allowing you to handle it yourself. Would that solve your problem?

@maiha
Copy link
Contributor Author

maiha commented Feb 15, 2018

Yep. There are two use cases.

  1. using natural keys
  2. manual seeding data with specified ids for some reasons

Here, I'm now facing the former problem. And I've already supported natural keys in my fork. It works with READ and UPDATE but not with INSERT because it seems granite doesn't respect new record well as I said in this issue.
https://github.com/maiha/granite-orm/tree/natural-key

My main concern is just using natural keys. And this issue is one method to solve it. So any other solutions are welcome! 😄

@drujensen
Copy link
Member

@maiha Your solution is what was recommended by @faustinoaq . I think its a good start. Go ahead and submit a PR. We will get your changes merged. Thanks.

@Blacksmoke16
Copy link
Contributor

Any update on this? Having support for natural keys is pretty important in my use cases.

@faustinoaq
Copy link
Contributor

This solves INSERT problem in #143 and will also help introduce natural keys.

^ @Blacksmoke16 see more: https://github.com/amberframework/granite-orm/pull/153

@maiha
Copy link
Contributor Author

maiha commented Mar 26, 2018

@Blacksmoke16
It seems that it takes time to merge at this repository.
FYI: So, I am using my fork and natural-key is working in amber-0.6.7 with the fork.

  granite_orm:
    github: maiha/granite-orm
    branch: master

@drujensen
Copy link
Member

@maiha is this still a WIP?

@maiha
Copy link
Contributor Author

maiha commented Mar 26, 2018

No. I've completed in my fork.
But since this depends on #154 , I'm waiting for it to be merged first. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants