Syntax for persisting models in Guacamole

[Martin Honermeyer]

Hi guys,

just started playing with Guacamole. The syntax for persisting models seems a little weird to me:

* Collection::save creates a new entry in the collection
* Collection::replace updates an existing entry

I would have expected something like the following:

* ::create creates a new entry
* ::replace updates an existing entry
* ::save checks for #persisted? on the model and calls ::create or ::replace accordingly

The first two methods could even be made private. That would make much more sense to me (and be more ActiveRecord-like). Is there a (ArangoDB specific?) reason it is implemented the current way?


Regards
Martin

[Lucas Dohmen]

Hi Martin,

Thanks for your feedback :) I'm open to renaming 'save' to 'create' if that makes it easier to understand :) What do you think, Dirk?

About creating/replacing:

In ArangoDB you can choose your key (which is part of the ID) if you like, or you can let ArangoDB choose one for you:

If you have a collection "my_stuff" and create a document in this collection with the key "hey", then this will result in the ID "my_stuff/hey",

if you do not choose it, it will be "my_stuff/random_generated_key".

When you replace a document, you can't change the ID. Therefore I thought that the "create" method could take an optional argument to

determine the key of the model. This would not be possible for a united method I think.

I'm also not sure in which situation the "save" convenience method helps: In my experience you always know if the model you want to put

into the database needs to be created or replaced. Do you have an example for a situation where this would be useful? :)

Or is this just to make the transition easier (which is absolutely valid in my opinion)?

Best Wishes,

Lucas

--
You received this message because you are subscribed to the Google Groups "Ashikawa ArangoDB" group.
To unsubscribe from this group and stop receiving emails from it, send an email to ashikawa+u...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

[Breuer Dirk]

Hi,

> Thanks for your feedback :) I'm open to renaming 'save' to 'create' if that makes it easier to understand :) What do you think, Dirk?

That's a good catch. I have no problem renaming it to 'save'.

>
> About creating/replacing:
> In ArangoDB you can choose your key (which is part of the ID) if you like, or you can let ArangoDB choose one for you:
> If you have a collection "my_stuff" and create a document in this collection with the key "hey", then this will result in the ID "my_stuff/hey",
> if you do not choose it, it will be "my_stuff/random_generated_key".
> When you replace a document, you can't change the ID. Therefore I thought that the "create" method could take an optional argument to
> determine the key of the model. This would not be possible for a united method I think.

But should the key not be set in the model? Why do you need to pass it as an argument to the `create` method? And if that's the case I don't see any problems with using a unified method. And even if that would not be possible, we could still leave the `create` method public. Providing a custom key will maybe not that convenient in the first place and thus it will not hurt using another method in this case.

> I'm also not sure in which situation the "save" convenience method helps: In my experience you always know if the model you want to put
> into the database needs to be created or replaced. Do you have an example for a situation where this would be useful? :)
> Or is this just to make the transition easier (which is absolutely valid in my opinion)?

Is it necessary for the API user to know if the record was created or replaced? Sure, there are cases you want that information but for those we could just provide methods like `was_replaced?` or `was_created?`. What do you think?

Best regards
Dirk

[Martin Honermeyer]

Am Dienstag, 3. Dezember 2013 23:02:31 UTC+1 schrieb Dirk Breuer:

Hi,

> Thanks for your feedback :) I'm open to renaming 'save' to 'create' if that makes it easier to understand :) What do you think, Dirk?  

That's a good catch. I have no problem renaming it to 'save'.

 

You mean to 'create', right? ;) We should really do that, because 'save' is pretty vague in this context. 'save' is more like "put it in the database, I don't care how".

> I'm also not sure in which situation the "save" convenience method helps: In my experience you always know if the model you want to put
> into the database needs to be created or replaced. Do you have an example for a situation where this would be useful? :)

Yes. It is a "find_or_create_by" situation (known from ActiveRecord). In my case, I implemented a method ::find_or_initialize_by_full_name on my collection which queries for an existing record or initializes a new one otherwise. Either way, it returns a model object. The model is then updated with new data. In order to persist the changes, at the moment I have to use "model.persisted? ? Collection.replace(model) : Collection.save(model)", which seems a bit unnecessary. On the other hand, I understand that Guacamole implements the "Data Mapper" pattern which propagates a clear separation between a model and its persistence logic (the collection), both being quite "dumb" entities. But I think adding a bit of intelligence in the save method helps a lot for convenience in these cases. Best regards Martin

[Lucas Dohmen]

Ok, let's rename that method :)

https://github.com/triAGENS/guacamole/issues/29

Currently the existence of a key is used to determine, if a model is persisted or not. We can of course change this and

introduce a new instance variable to save if this model is persisted or not. This would default to false (for models created

by the user) and be set to true by the Collection when it came from the database. I think that keeps the separation between

the concerns of the Collection and Model.

But would that be useful in your case? Because if I understood you correctly, you would like to create a Model on your own

(which would – in the implementation as described above – mean that `persisted?` returns false. So the `save` method would

ask the model, if it is persisted, it would say "No, I'm not persisted" and therefore call the `create` method.

And this is not what you would want it to do, right?

Another way to do this would be to always just find or create by the key. But with this way you could overwrite entries in the database

by accident, because you choose a key that has been chosen before.

Is there a different way?

Best Wishes,

Lucas

[Martin Honermeyer]

Hi,

Am Mittwoch, 4. Dezember 2013 14:39:10 UTC+1 schrieb Lucas:

Currently the existence of a key is used to determine, if a model is persisted or not. We can of course change this and

introduce a new instance variable to save if this model is persisted or not. This would default to false (for models created

by the user) and be set to true by the Collection when it came from the database. I think that keeps the separation between

the concerns of the Collection and Model.

Sounds good to me.
 

But would that be useful in your case? Because if I understood you correctly, you would like to create a Model on your own

(which would – in the implementation as described above – mean that `persisted?` returns false. So the `save` method would

ask the model, if it is persisted, it would say "No, I'm not persisted" and therefore call the `create` method.

And this is not what you would want it to do, right?

No, a new model is only initialized if it does not yet exist in the collection. Here is an example I would like to work:

class RepositoriesCollection
  include Guacamole::Collection

  def self.find_or_initialize_by_full_name(full_name)
    by_example(full_name: full_name).first || Repository.new(full_name: full_name)
  end
end

repository = RepositoryCollection.find_or_initialize_by_full_name('my repo')
repository.some_field = 'some value'
RepositoryCollection.save repository

So calling the new ::save method with the returned model will do they right thing in both cases.

Another way to do this would be to always just find or create by the key. But with this way you could overwrite entries in the database

by accident, because you choose a key that has been chosen before.

I do not think that would be desirable. The proposed approach should work fine

Cheers
Martin

 

[Lucas Dohmen]

Hi,

Thanks again for your pull request :)

Ok, then let's go with this solution for determining if a model has been persisted :) I created the according issue:

https://github.com/triAGENS/guacamole/issues/31

Best Wishes,

Lucas