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

Update - Ecto.Adapters.SQL.Connection #46

Open
Danwhy opened this issue Feb 14, 2019 · 5 comments
Open

Update - Ecto.Adapters.SQL.Connection #46

Danwhy opened this issue Feb 14, 2019 · 5 comments
Assignees
Labels

Comments

@Danwhy
Copy link
Member

Danwhy commented Feb 14, 2019

part of #38

https://hexdocs.pm/ecto_sql/Ecto.Adapters.SQL.Connection.html#c:update/5

Our update implementation needs to work in much the same way as our insert, but it also needs to compare the newly generated cid to the old one, and not do anything if they are the same as it means the data is unchanged. If they are different, it needs to be added to the row as the previous_cid.

@Danwhy Danwhy added the enhancement New feature or request label Feb 14, 2019
@RobStallion RobStallion added question Further information is requested discuss labels Feb 15, 2019
@RobStallion
Copy link
Member

Are we planning on turning the entire map/struct into a cid? If so how do we plan on comparing the new cid to the old one? Each CID should always be unique as the struct passed in will contain timestamps.

Could we compare the new params to the old params instead?

@Danwhy
Copy link
Member Author

Danwhy commented Feb 15, 2019

It depends on when we actually create the cid I guess. When do the timestamps actually get added? I would think it's not until the item is actually inserted into the database, at which point the cid would already exist

@RobStallion RobStallion removed the question Further information is requested label Feb 15, 2019
@Danwhy Danwhy self-assigned this Feb 20, 2019
@Danwhy
Copy link
Member Author

Danwhy commented Feb 20, 2019

Re: @RobStallion's comment above. It turns out that Ecto already compares the parameters in the changeset function, and only passes through those that have changed to the Adapter's update function. If none of them have changed, the function doesn't get called, so we actually have no need to compare the cids before updating.

@Danwhy
Copy link
Member Author

Danwhy commented Feb 20, 2019

Some of the issues I've encountered so far:

If passing a schema to the update function, an error will be thrown if the schema does not have a primary key. This means we have to require users to define the cid primary key in their schema. Not ideal, but we can write a macro to help: see https://hexdocs.pm/ecto/Ecto.Schema.html#module-schema-attributes for an example

To allow for the cid to be generated on the backend, we need to pass autogenerate: true when we define it. Autogenerated ids cannot have a primitive type such as :string. I'm able to insert a binary as the id value, but I get an error when I try to call the update function:

** (Ecto.ChangeError) value `"zb2rhn1C6ZDoX6rd"` for `Alog.TestApp.User.cid` in `update` does not match type :binary_id

Looks like we may need to define the loaders functions:
https://hexdocs.pm/ecto/Ecto.Adapter.html#c:loaders/2

@Danwhy
Copy link
Member Author

Danwhy commented Feb 28, 2019

I've managed to get this function fully working, it creates a new row in the database with the updated fields and a new cid.

The only issue is the return value of the update function. The returned struct seems to be built separately from what is actually inserted into the database, and it's not being built correctly; the cid remains the old value, while the updated fields are updated. I need to take a look at how the struct is returned and see what we can do.

Danwhy added a commit that referenced this issue Feb 28, 2019
@Danwhy Danwhy mentioned this issue Feb 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants