Check persisted using real existence (instead of id presence)

[aderyabin]

Currently if the entity has id,

user = User.new({id: 123, login: 'aderyabin'})

there are no way to persist this entity because .create returns nil because id are present and .update fails because record in database with this id are not exists.

I suggest to check on persistance by real query to repository. If repository do not have entity with the equal id then and entity are not persisted.

It easy to display by example/test:

user = UserRepository.create(User.new(name: 'Don', id: 25))
UserRepository.all.must_include(user)

// cc @AlfonsoUceda, you asked me to prepare use case and PR. :)

[AlfonsoUceda]

Thanks @aderyabin for your PR. I've been reviewing your PR and we can't make a find every creation, it isn't inneficient.

We should remove the unless or create a way for the entity knows the custom id. Cc @lotus/core-team

[AlfonsoUceda]

One thing, do you need the id? A workaround you can use is create another attribute instead id for your custom id

[aderyabin]

Yes, as discussed before I can use custom id field. But it is a compromise.

In general I don't see any problem to use find on creation. It's like find_or_create from Rails. It's an effortless operation for database.

[runlevel5]

@AlfonsoUceda I have no problem with this approach at all, so it is 👍 from me

[AlfonsoUceda]

@joneslee85 but for that we'd have find_or_create for example, find every creation on create method it's inefficient.

[runlevel5]

@AlfonsoUceda the other better way is to ensure that attributes that map to primary_key is not writable by public.

[aderyabin]

@joneslee85 . I think that is not good idea too because .persist determs method (.create or .update) by primary_key presence and lotus has great test when primary_key pointed manually https://github.com/lotus/model/blame/master/test/entity_test.rb#L163

[runlevel5]

cc @jodosha what do you think?

[jodosha]

@aderyabin Thank you, but I agree with @AlfonsoUceda It's inefficient. The example that you mentioned about AR find_or_create doesn't apply to this context.

In that case a developer wants to find a record or otherwise create it, they are aware of the cost of the operation. Here instead, we're adding a perf tax which is unnecessary and unexpected by devs.

I also won't suggest to setup id by hand, because you can run into this error in case of creation:

ERROR:  duplicate key value violates unique constraint "users_pkey"

..or update the wrong record by mistake in update.

[aderyabin]

@jodosha
Well, I have a real task.
I receive users from API in JSON format from different sources.
Each user data like this

{
 id: 123,
 login: `aderyabin`
}

id is unique.

I have to save this data and don't duplicate users.

I tried this code

class User
  include Lotus::Entity
  attributes :login
end

u = User.new(id: 123, login: 'aderyabin')
UserCollection.permit(u)

And it doesn't work, because Lotus think that entity is permitted (because #id present) and Lotus wants to update a record and raise exception.

This PR check entity is permitted really (instead of virtual).

[jodosha]

@aderyabin Fine. I'm determined to find a solution to your problem, but this isn't the way to proceed. It has a perf cost and introduces race conditions with the database.

Can you please open a new PR with a different purpose?
It should:

1. Remove persist both from adapter and repository
2. Remove the check for id in create and update

This should give more control on id. Thank you! 👍

[aderyabin]

@jodosha
Ok, no problem.
What is the preferable logic instead of raising NonPersistedEntityError exception if id is not present? Return nil?

[aderyabin]

Lotus has many adapters, some can raise PG::UniqueViolation: ERROR, some be silent by default. I think that behavior should be equal for all.

[jodosha]

@aderyabin Okay, replied while in a hurry. Sorry.

RE NonPersistedEntityError: if it doesn't interfere with the new "take control of your id" policy, please leave them as they are.

RE Unique constraint: if it's easy to implement for memory adapter, please go ahead. 👍

[pascalbetz]

My 2 cents (might be to late as there is already a PR waiting, sorry)

• If a record is persisted or not should not be determined by presence of id but trough a separate attribute as @AlfonsoUceda suggested, then one can decide if it is an INSERT (with or with PK supplied) or an UPDATE (always with PK supplied in the WHERE clause)
• Calling find on every save is not what i would expect (even though in many cases the performance impact might be neglectable)
• There is a difference between a PK and a value which is constrained to be unique: in @aderyabin sample with the JSON API, i would store the :id attribute from JSON as external_id attribute in the DB, not as PK. Probably. Don't know the whole story there.

[jodosha]

If a record is persisted or not should not be determined by presence of id but trough a separate attribute as @AlfonsoUceda suggested

@AlfonsoUceda @pascalbetz Can you please explain the idea? IMO id is reliable for that, because we force it to be the "identity" of an entity. Even if the database has a different field, with the mapper we solve this issue.

Ref #258

[pascalbetz]

@jodosha

There is a difference between id assigned and persisted. More so if the id can be provided from the outside (which apparently is a requirement). An entity is persisted IFF it was previously saved or previously loaded from the repository, not if its identity is non-nil.

If i create an entity:

foo = Foo.new(id: 12, name: 'foo')

then you can not tell if you need to save or update that entity. If there is an additional attribute (maybe persisted?) that is false when entity was created with new and true when entity is loaded from repository or saved to repository, then that would solve that problem.

Does that make sense?

[thhermansen]

I do think I understand @pascalbetz example and I have a similar situation in my application where I use Lotus: In our application logic we generate uuids and use that as the identity for entities. In our database we don't use any sequential assigned id; we use the uuid in postgres as our primary key.

When we want to persist new records and setting an entity's ID to the assigned uuid we have a confused repository not wanting to create that record, as it believes it is already created. We are currently using #258 which allows us to create and persist the record.

[jodosha]

@thhermansen Hello, I see your point and #258 will be merged soon. /cc @aderyabin

@aderyabin I understand the technical difference between assigned id and persisted record. However, I'm still thinking that it's a perf tax that we don't want to add to the framework, and it's a race condition, because two threads can assign the same id, and because the check and the writing aren't atomic, we'll have a problem then.

If you instantiate an entity Foo in the example above, by passing the id, you can tell from the current context if it's persisted or not, because you haven't involved a repository yet.

What I'm trying to say is that, we should rely on conventions to avoid unnecessary overhead.

/cc @pascalbetz

[pascalbetz]

@jodosha

If the ID is assigned by the client, then it's in his responsibility to make sure he does not use the same ID twice and also to deal with errors related to that. So no need to run a check. Just let the thing crash.

Yes, you can tell from the context in that case. And you can then make sure you call the right method (INSERT vs. UPDATE). But IMHO it is something that is dealt with in the framework otherwise the if/else code will be in many places in the client code instead of just the framework. Also it is something that the entity should know (in case you need to pass it to other pieces of code): have i been saved or not.

[jodosha]

But IMHO it is something that is dealt with in the framework otherwise the if/else code will be in many places in the client code instead of just the framework.

Well, if you're assigning id, you know that create is the operation to call, without the need of an if. Am I missing something?

[pascalbetz]

No. Still i think that this is something the entity should know, not the context the entity "lives" in.

[jodosha]

@pascalbetz The only way to know for sure is to do the database check that @aderyabin suggested. Which I assume is out of scope.

[pascalbetz]

@jodosha

Yes, to "know for sure" i agree. Because there might be concurrent processes. But for the use cases i had where an external ID is involved this was not an issue because the source of the external ID (usually another DB or a UUID service) made sure that there is no clash.

My proposal is to add another attribute _persisted which is set to true when entity is loaded from repository and after sucessful create. This should do the trick for most situations and would make sure that

foo = Foo.new(id: 12, name: 'foo')
foo.persisted?

gives the right response (false in the case above).

[jodosha]

@pascalbetz Ok, we should hold on with this feature, because I want to experiment with a different way to persist data:

foo = FooRepository.create({id: 23, name: 'foo'})

This will solve this issue and also this discussion reported here: https://discuss.lotusrb.org/t/change-the-behavior-of-repository-create-persist/117