Not clear when to call save() vs. saveAll() when updating a doc with relations

[mindjuice]

I have a Class model with the following definition:

var Class = thinky.createModel('Class', {
  id:       type.string().options({enforce_missing: false}),
  name:     type.string(),
  isActive: type.boolean(),
  schoolId: type.string()
});

// Relationships
Class.belongsTo(School, 'school', 'schoolId', 'id');

Class.hasAndBelongsToMany(User, 'teachers', 'id', 'id');
Class.hasAndBelongsToMany(User, 'students', 'id', 'id');

If I set schoolId to a new value, followed by a save(), then the next time I do a getJoin() my school is properly populated with the new value (and I see it in RethinkDB Data Explorer). If I instead do a saveAll() though, the schoolId field does not get updated, even if I pass in {school: true}.

On the other hand, if I set a new array for students (as described in #184), which is a many-to-many relationship, then I have to use saveAll() or the relationship does not get updated (as expected).

This is rather cumbersome, as I have a generic PATCH endpoint that accepts partial document updates and applies them. In order to make this work now, I have to figure out which type of field I am updating and then call save() or saveAll() as appropriate. It seems that calling saveAll() should just always work.

As with issue #184, I need to be able to save string ids, but it would be okay if I needed to just set {school: someSchoolIdString} instead of {schoolId: someSchoolIdString}. Ideally though, just setting {schoolId: someSchoolIdString} in saveAll() seems like it should also work.

Thoughts?

[neumino]

So save will save this document, basically all the fields defined in the document.

saveAll({key1: ..., key2: ..., keyn: ...}) will look at the joined documents in key1 to keyn, populate the foreign keys, and save this document and the one stored in key1 to keyn,

This means that if you set schoolId and use save, the relation is saved. If you call saveAll, we will looked at the joined school, see that there is not, and therefore consider that there is no joined school and clean the field schoolId. saveAll always work if you provide the joined documents. saveAll has to be able to delete the relation which is why it doesn't operate on foreign keys only.

Now the workaround if you want to add a relation without having the documents is to set the foreign keys yourself. For belongsTo you can just set the key and use save. For hasOne and hasMany, you can set use JoinedModel.get(joinedDoc.id).update({rightKey: doc.leftKey}) to add a relations. For hasAndBelongsToMany, you can just provide the new keys and call saveAll.

There is currently not way to just save just relations given keys. I can imagine two solutions:

. Now saveAll will delete the foreign keys as long as the value for the foreign key is missing. Meaning (in the common case) when the joined document is undefined or null. We could maybe for undefined not delete the foreign key and do it with only null. That requires some work though. 2. Another solution would be to let people provide just the keys for all relations and not just hasAndBelongsToMany.

I kind of prefer 2 over 1 mostly because it's backward compatible (and easier to implement I think).

Let me know if you have more questions

[mindjuice]

Thanks, that description makes sense. Just to confirm my understanding, if I do the following (pseudocode syntax):

var doc = Class.getJoin({school: true});
doc.schoolId = someId;
doc.saveAll({school: true});

Then what you are saying is that saveAll({school: true})will see the document reference for school that was retrieved from the getJoin({school: true}) and use it to replace the schoolId, and the new schoolId that I just set will be overwritten.

But if I were to do the following, then schoolId would be properly updated (pseudocode syntax):

var schoolDoc = School.get(someSchoolId);
var classDoc = Class.getJoin({school: true});
classDoc.school = schoolDoc;
classDoc.saveAll({school: true});

Solution 2 seems like the right one to me. If you can provide string keys for hasAndBelongsToMany, then you should be able to provide keys for all other relations.

[neumino]

Yes, what you described for schoolId is spot on.

Implementing solution 2 shouldn't be hard. Thanks for the feedback @mindjuice!

[AshHeskes]

This would be really useful to have the ability to assign key strings or classes for all relations.

I have a case where I would need both kinds.

var Customer = thinky.createModel({

    name : String,
    email : String  
});

Var App = thinky.createModel( {

    owner_id : String,
    name : String
} );

var ApiKey = thinky.createModel( {

    owner_id : String,
    role : String,
    revoked : Boolean
} );

/*
    Customer Relations.
*/
Customer.hasMany( App, 'apps', 'id', 'owner_id' );

/*
    App Relations.
*/
App.belongsTo( Customer, 'owner', 'owner_id', 'id' );
App.hasOne( ApiToken, 'public_api_key', 'id', 'owner_id', );
App.hasOne( ApiToken, 'private_api_key', 'id', 'owner_id' );

/*
    ApiToken Relations.
*/
ApiToken.belongsTo( Customer, 'owner', 'owner_id', 'id' );

// This isn't actually implemented in my code, but I'm interested to know what would happen in the case
// of two belongsTo relations on the same field of a model? As far as I can tell one should overwrite the
// other right. Since belongs to relations are stored on the model itself not in a separate table? Although
// this would be a useful feature. Especially in the case here of "generic" `ApiTokens` that can be issued
// and assigned to customers, apps, third parties, password resets etc etc. But maybe I should move this
// question to a separate issue?
ApiToken.belongsTo( App, 'owner', 'owner_id', 'id' );

/*
    A controller creating an instance of `App` would look like.
*/
function AppCreateController( request, response ) {

    var customer_id = request.params.customer,
        app_data = _. pick( request. body, 'name' );

    var app = new App( {

        // The customer already exists and we already have the id
        // from the request so we shouldn't need to fetch the document.
        owner : customer_id,
        name : app_data.name,
        private_api_key : new ApiToken( { role : 'app_private', revoked : false } ),
        public_api_key : new ApiToken( { { role : 'app_public', revoked : false } } )
    } );

    /*
        Should work but instead fails because `owner` is an id string.
    */
    return app.saveAll( { owner: true, private_api_key : true, public_api_key : true } );
}

[neumino]

You can create your app and save it first and the call use addRelation http://thinky.io/documentation/api/document/#addrelation

[AshHeskes]

Sure I could also just fetch the customer first. I just wanted to give another use case for allowing Strings in saveAll calls instead of Classes.

Is there any opposition to that from your perspective?

[AshHeskes]

Also what are your thoughts on BelongsTo on the same field mapping to different Models?

i.e. the case in the example for this code.

ApiToken.belongsTo( Customer, 'owner', 'owner_id', 'id' );
ApiToken.belongsTo( App, 'owner', 'owner_id', 'id' );

[neumino]

For belongsTo you can directly pass the string to the foreign key. For hasAndBelongsToMany, you can also pass a value instead of a document (the value of the foreign key) For hasOne and hasMany you currently cannot do it.

You should probably use addRelation since it's not more expensive (you don't have to retrieve the document first), and you have to hit the joined document as the key is stored there.

[neumino]

Also on a side not, I'm thinking about deprecating saving a hasAndBelongsToMany relations with values as there is now addRelation.

[AshHeskes]

Also after testing my code it appears there's an error caused by.

App.hasOne( ApiToken, 'public_api_key', 'id', 'owner_id', );
App.hasOne( ApiToken, 'private_api_key', 'id', 'owner_id' );

When doing a App.get( 1 ).getJoin({ public_api_key: true }) there's a ReQL error thrown because the ApiTokens table matches two documents who's 'owner_id' field matches the App.

Clearly the mapping here is being done purely on 'owner_id' and the leftKey of the hasOne serves no purpose other than the key to populate in the application logic.

Although just from a logical standpoint I think you can see how the above code "makes sense".

On your note regarding dropping hasAndBelongsToMany in favour of addRelation. Can you explain how addRelation differs in order to accomplish the same task. As in is addRelation basically an adhoc way of describing arbitrary relations of any type? How does that work with multiple addRelation calls on the same field vs a singular (you could probably do with improving the docs on this method)?

In my example case and in your documentation hasAndBelongsToMany can be used to accomplish "different types" of relationships on the same model.

e.g.

Customer. hasAndBelongsToMany ( ApiTokens, 'api_tokens', 'id', 'id', { type : 'customer' } );
Apps. hasAndBelongsToMany( ApiTokens, 'api_tokens', 'id', 'id', { type : 'app' } );

How would the same be accomplished with addRelation?

[AshHeskes]

If my hunch on addRelation being used for arbitrary relations is correct I'm not sure that I'm a fan of it. From a maintainability standpoint that can get messy and complicated very quickly since there's no singular place in which one can describe all of a models "relations". Unless addRelation was made part of a schema instead of part of the query.

If I'm honest. my initial instinct of what addRelation did was to "force" the saving of a predefined hasOne, hasMany, belongsTo, hasAndBelongsToMany relationship. Not for creating arbitrary relations.

If you want my honest opinion. I'm wondering whether it makes sense to have all relation types stored in their own table, or use some secondary index hack to allow for multiple relations between models.

e.g.

App. hasMany ( ApiTokens, 'private_api_token', 'id', 'owner_id' );
App. hasMany ( ApiTokens, 'public_api_token', 'id', 'owner_id' );
Customer. hasMany ( ApiTokens, 'api_tokens', 'id', 'owner_id' );

Would internally create a table mapping ApiTokens > to their owner via the owner_id and model Class.

Also it might be conceivable to make this mapping directly on the Model with a hashed secondary index that is a hash of owner_id + other_model_key + other_model_class.

Would I be right in thinking it might be possible to use that hack for all relation types? Would it be cleaner than a separate table? more efficient to query? Would it also be easier to maintain?

Please take all of this with a grain of salt. I'm brand new to thinky and Rethinkdb so I could just be completely misunderstanding everything :)

[neumino]

1. You are misunderstanding addRelation. It doesn't create a new relation between two models, but a new relation between two documents. Typically if you have two saved documents A and B and want to join them, you could use addRelation.
2. Your example with the relations between App and ApiTokens doesn't work and this is expected. You speficy twice the same foreign key. Meaning that this field owner_id is supposed to store two apps'id. Thinky uses the same mechanisms as you would in SQL. There's some doc about how relations are built here: http://thinky.io/documentation/relations/

What you probably want is probably

App. hasMany ( ApiTokens, 'private_api_token', 'id', 'private_owner_id' );
App. hasMany ( ApiTokens, 'public_api_token', 'id', 'public_owner_id );
Customer. hasMany ( ApiTokens, 'api_tokens', 'id', 'customer_id' );

[AshHeskes]

I understand what you're saying and I know I could do it that way.

I guess I was just suggesting that it would be nice for the foreign key to not be restricted to one type of relation and instead include the table_name and local_key as part of the relation. I was thinking that allows for much more flexibility and "uniformity" than having data stored where you're creating non uniform foreign_key fields to get around it. Using a secondary index to store and lookup the relation might be an approach to doing it.

What are your thoughts on that?

[neumino]

It doesn't really seem like a good idea mostly because it means that you can't easily create the opposite relation.