Objection.js: upsertGraph method

[koskimas]

I'm proposing an update equivalent for insertGraph and eager operations that allows one to patch a model and all it's relations using a single method call. upsertGraph would

• Update the root model
• Go through all relations and
  
  
  
  • Remove related models that no longer exist
  
  
  • Add new related models
  
  
  • Update existing if some value has changed
  
  
• Call updateGraph recursively for relations

This could be done in batches to some degree. For example we could

• delete all removed models using a single delete where in operation per table
• insert all new models using a single batch insert per table.

What do you think everyone? Please vote for this if you think this is a good idea. All comments and suggestions are welcome!

[elhigu]

Sounds awesome! Any ideas how to check exactly if related models has changed values?

[lvffilho]

Nice one! I am trying to develop something like this. First idea was query object again by ID and DIF both json, and with relations names run the operations (delete, insert, update)

[EdsonBueno]

Any idea on when we will be able to use it? Really needing it!

[koskimas]

Next year. I'm leaving on a trip in a couple of weeks, and I won't be back until next year (I won't have internet access there). And during the next couple of weeks, I won't have much time for objection.

[drew-r]

Looking forward to this @koskimas .
Would this support filters / options to patch relations rather than removing models than dont exist?

[mceachen]

This sounds great. You might want to call this upsertGraph rather than updateGraph (because update to the SQL pedant means change, not add, rows to the database).

[koskimas]

@drew-r Yes, my current vision is that you can choose any combination of patch/insert/delete. The implementation will be highly configurable since there are so many different use cases for this kind of operation.
@mceachen That's a good idea. Let's call it upsertGraph.

[koskimas]

I've started writing this three times and ended up saying "**ck it, it's too hard" followed by git reset --hard. upsertGraph has soooo many problems to be solved and things to consider that I'm beginning to wonder if it can be implemented in a clean and useful way.

Problem 1

We would need to detect existing models from the fact that they already have id's present. There is no other meaningful way to determine if a model already exists in the database. For example checking that all properties are equal doesn't mean anything. There can easily be two rows with the exact same values. Well ok, we could just decide that existing models are detected from id field. Then what should this query do?

Person
  .query()
  .whereIn('id', [1, 2, 3, 4, 5, 6])
  .upsertGraph({
    id: 7,
    firstName: 'Jennifer'
    pets: [{
      {id: 1, name: 'fluffy'},
      {name: 'Coco'}
    }]
  })

We are updating multiple rows, but clearly the update graph is only meaningful for Jennifer. How about this then:

Person
  .query()
  .whereIn('id', [1, 2, 3, 4, 5, 6])
  .upsertGraph({
    firstName: 'Jennifer'
    pets: [{
      {id: 1, name: 'fluffy'},
      {name: 'Coco'}
    }]
  })

Now we could update 1, 2, 3, 4, 5 and 6 to have firstName='Jennifer' but what should we do with their pets? pets is a HasMany relation and we cannot relate the existing pet fluffy to more than one place.

Now we could decide that upsertGraph is special and ignores the where clause of the query. It always updates stuff by their id property. This doesn't fit well with rest of objection. For example this:

jennifer
  .$query()
  .upsertGraph({
    firstName: 'Bradley'
  })

What would this do? Based on what we have discussed here, this should create a new row Bradley since there is no id. But the query is an instance query for jennifer.

We could make usertGraph really special and just allow it to be used after static query and $relatedQuery and throw if there are any where conditions or other filters in the query since they would not work as expected.

Problem 2

If we detect existing models by id, then what should we do if the model doesn't exist in a relation? For example:

Person
  .query()
  .upsertGraph({
    id: 1,
    firstName: 'Jennifer'
    pets: [{
      {id: 1, name: 'fluffy'},
      {name: 'Coco'}
    }]
  })

what if Jennifer doesn't have a pet by id 1 related to it? Should we add it with a new id? Should we throw an exception? If we do either of these options we need to fetch all the models with id's before the actual upsert and check that they are related to correct people.

How about this then:

Person
  .query()
  .upsertGraph({
    firstName: 'Jennifer'
    parent: {
      id: 5
    }
  })

we need to insert Jennifer since it doesn't have an id, but then there is an existing parent by id 5. Should we relate the Person with id 5 for jennifer? What if it's already related to some other row? Assuming that's a HasOne relation relating it to Jennifer would make it doubly related and break the HasOne condition. Should we unrelate 5 from its previous owner? That would be confusing wouldn't it?

The problem list goes on. If I just pick a solution for each of these I will get 9000 issues about the way things work. If I make everything configurable, this will never be ready. Sigh... Maybe I'm overthinking the whole thing.

[fl0w]

Would it simplify upsertGraph if it's very strictly defined/specced? I.e. be very defensive of it capabilities.

The problem I see with these type of high level functions is the magic (my opinion). They're great as an USP, but at the end of the day maintaining these dragons is hard (unmanageable?).

Problem 1
if id exists, It has to be an update - if no rows were updated, throw. Since it would throw, I would personally never run upsertGraph without transacting. Maybe that should be enforced as well.

Regarding pets relation as HasMany, personally I wouldn't mind objection throwing because that's faulty logically. Having a Model which depends on another (1) Model and trying assign it to multiple instead makes no sense practically.

Problem 2

If we detect existing models by id, then what should we do if the model doesn't exist in a relation?

I don't think it's irrational to throw. An id in object usually indicates an update - and I think it's too specific expecting objection to magically conclude an insert or update depending on X and Y. As a side effect, it's easy to document and explain/reason about as well e.g: "By passing an object with an id (specifically Model.idColumn) to upsertGraph, objection.js assumes to update and expects appropriate value returned from database (e.g. num rows updated = 1). Objection will throw if num rows updated isn't equivalent".

what if Jennifer doesn't have a pet by id 1 related to it?

I would personally assume, based on the example, that fluffy gets updated to relate to Jennifer, and Coco gets inserted and related to Jennifer. If the update returns 0 rows updated, throw.

we need to insert Jennifer since it doesn't have an id, but then there is an existing parent by id 5. Should we relate the Person with id 5 for jennifer? ... [etc]

I don't have brain to understand this one so I have no comment :(

[venelin-mihaylov]

@koskimas
Feedback:
Upsert a model along with its relations can be divided to:
a. create/update model
b. upsert relations (the most common use case IMO)

So, example db schema, input and proposed API ...
schema:

person (id, firstName) (relation owner)
pet(id, personId, name)

input:

{
    id: 1,
    firstName: 'Jennifer',
    pets: [{
        id: 1
    }, {
        // no id, create related record
        name: 'Zeus'
     }]
}

API:

// create
Person.insertAndFetch(input).then(r => r.upsertRelated(input))
// update
Person.updateAndFetchById(id, input).then(r => r.upsertRelated(input))

b) is the most common use case that is not present in most ORMs. I myself am interested in that use case.
What you would say to implement only upsertRelated() ? Only as an instance method?

Deletion:
What happens when a pet is not present in the input, but is present in the datatabase?
option 1. unrelate() the pet from the parent
option 2. delete the pet altogether
For this IMO a configuration option is appropriate.

I guess you've thought about the needed logic, but here are my 2 cents anyway:

1) keyed many-to-one relation
input:
keyed 1:N relation, owner is 1 side of relationship

{
    firstName: 'Jennifer',
    // one-to-many relation, so expect an array for the "many"
    pets: [{
      // primary key set,
      // pseudo code: recordExists ?  updateRecord : createRecord
      id: 5, 
      // no foreignKey (personId) is set
      // relate() the pet record to "person" record
      name:"Zeus"
    }]
}

1. keyed belongsTo, create new foreignKey record
   input:

{
   id: 1,
   name: 'Zeus',
   person: {
       // no id, create person
       firstName: "Jennifer"
   }
}

2.b.3. keyed belongsTo, update existing person

{
  id: 1,
  name: 'Zeus',
   personId: 1,
   // foreign Key is p
   person: {
     id?: 1, // not really needed
     name: 'Jennifer updated'
   }  
}

Hope this helps...

[johannessjoberg]

Any news regarding this feature?

I agree with @fl0w . To make it manageable I think it will be important to make this feature strictly defined and opinionated. That will most likely results in a bunch of opinionated issues, but also a very useable feature!

[Gustav0ar]

looking forward for this feature

[koskimas]

__Now__ I'm going to sit my lazy ass down and write this! What has been said, cannot be unsaid (for the fourth time).

[koskimas]

First alpha version is almost done. One more night of coding and I'll push this into a branch for the bravest of you to test and comment.

[fl0w]

Awesome!

[koskimas]

For **ck's sake! I just hit a brick wall with my design. I knew this would happen with this one. Complete rewrite... 😭

[newhouse]

Hang in there and good luck!

[koskimas]

I just pushed the first draft of the upsertGraph method to master. I documented the tests so that they can be used as a reference at this point. There is still alot of work to do, but it should already work for the most common use cases.

I would be very grateful if some of you tested this for their own use cases and provide any feedback and suggestion.

[mceachen]

Well done!

I've found chai-subset to be helpful in my objection integration tests, so you don't have to do stuff like this. Also, kudos to writing out all the TODO test cases at the bottom.

[koskimas]

@mceachen Thanks for the tip! I'll switch over to chai.

[theogravity]

I was brave to test!

Having an issue with unrelate not working (?)

        .query(trx)
        .insertGraph({
        id: 'test-id',
        name: 'name-000',
        logoURL: 'logo-000',
       // profile is a user profile table
        profile: {
          email: '[email protected]',
          // address is stored as a json entry; is type object in the model
          address: {
            streetAddress: 'test-street-addr'
          }
        }
      })

Address has another field called `extendedAddress`. I decide to update that field (but I want to still keep `streetAddress`

User
        .query(trx)
        .upsertGraph({
        id: 'test-id',
        name: 'name-111',
        logoURL: 'logo-111',
        profile: {
          email: '[email protected]',
          address: {
            extendedAddress: 'extended-street-addr'
          }
        }
      }, { unrelate: true })

// the streetAddress gets removed

I guess unrelate is only for relations? Could there be a merge type option?

[theogravity]

Also an option to only do an update operation only and throw if it's trying to update an id that doesnt exist

Maybe a patchGraph type operation is more appropriate

[koskimas]

@theogravity Thank you for testing this!! yes, unrelate only works for relations. merge option for object columns could be a good idea! I'll write it down. It could be a bit difficult to implement, so that propably won't be the first thing I'll implement. I'm pretty sure worse bugs and missing features will be found soon 😀

Option to throw if nothing gets updated (invalid id, or id not found in relation) is an excellent idea! I'll do that next. Currently those either silently fail (root) or get related (relations). Maybe that should even be the default?

[theogravity]

Default.

For patching I noticed there was no validation if the id is missing (via patch and fetch by id) - would be great if it could make sure the id was present instead of having to do a manual check for it

[elhigu]

Would it be ok to validate only those fields which are patched and ignore required attributes validation altogether? I don't know how easy that is to implement with our validation lib though and still be allowing custom validators... and sorry for being off topic here.

[theogravity]

Maybe there should be a separate required patch attributes field (eg, I require an id only for the id param of patchAndFetchById)

[koskimas]

@theogravity please create a separate issue for the patchAndFetchById issue. This issue is only about upsertGraph.

[koskimas]

@theogravity upsertGraph now throws if an invalid id is given somewhere in the graph by default. The original relate behaviour can be enabled by giving a relate: true option.

[theogravity]

Thanks, I'll give it a try at work tomorrow, although my use-case is mainly patching compared to updating.

[theogravity]

It works! It was able to associate the user id in the profile.