Possible fix for validateUpsert

[ShaharHD]

Possible fix for #10 , #11
Maybe even #17 (someone can test?)
#12 will be solved once loopbackio/loopback-datasource-juggler#516 will be incorporated.

The solution I revised follow this:

On the init phase:

1. Check if there's a disableAllValidateUpsert override. in that case simply continue normally.
2. if no override, 1st check to see if the model is based off PersistedModel
3. if not, give the warning (modified to mention the disableAllValidateUpsert)
4. if yes, continue as normal

On the hook:

1. Try to use ctx.currentInstance (available in PersistedModel based models)
2. If exists, reuse the createdAt for the update.
3. If not exists, set createdAt to date()

[clarkbw]

This looks good at first pass, I'm just updating things and will rebase this on the changes to investigate further. Thank you.

[ShaharHD]

If you or anyone else have a better way to check if the Model is based on PersistedModel ... I would love to know :)
I was googling for a while, the try/catch solution was last resort.

[clarkbw]

PersistedModel

I believe you can look at the Model.settings.base value to see if it equals 'PersistedModel' but if something is based off a child of PersistedModel it might give the wrong result.

[ShaharHD]

I've added Model.settings.base check before the exist method for possible speed optimization.

[margagn]

I am new to Strongloop/Loopback and just tried loopback-ds-timestamp-mixin. When I start my service, I get "TimeStamp mixin requires validateUpsert be false. See @clarkbw/loopback-ds-timestamp-mixin#10". Will this check in fix this issue or we must have validateUpsert set to false?

Thank you!

[ShaharHD]

@margagn the pull removes the need to have validateUpsert set to false on PersistentModel based models

[ShaharHD]

@clarkbw anything I can help with to get the version 3.2.3 out?
Maybe without your extended branch for ES6?

[mrfelton]

This doesn't appear to work for me.

PUT to updateAttributes works as expected (created date not modified).
However, POST to upsert results in the created date being modified along with the modified date.

Note: My models are based off a subclass of PersistedModel (not off PersistedModel directly).

[ShaharHD]

@mrfelton few questions:

1. Do you have any embedded models inside the model you are trying to save?
2. Please turn on the debug flag and paste here the init console log and also the actual log from the upsert event.

[ebarault]

Hi,
This does not seem to work for embedsMany relations:

Model Trip:

  "relations": {
    "passengers": {
        "type": "embedsMany",
        "model": "Passenger",
        "property": "_passengers"
    }  }

Model Passenger:

"name": "Passenger",
"base": "Model",
"idInjection": false,
"options": {
  "validateUpsert": true
},
"properties": {
    "passengerId": {
        "type": "string",
        "id": true,
        "generated": false,
        "required": true
    }
},
"mixins": {
        "TimeStamp" : {
            "createdAt" : "createdAt",
            "updatedAt" : null
        }
    },
"validations": [],
"relations": {}
}

}

Now, using prototype_updateById_passengers on Model Trips to update a singler Passenger instance, the createdAt date is refreshed anytime

[ShaharHD]

@ebarault I had the exact same issue. It's related to a totally different bug on juggler which events are not triggered correctly for embedded models.

The fix for this right now (I have the exact same issue) is to save the father model and then add the embedded models using the father model id.
like:

fatherAPI + '/' + body.id + '/passengers'

This way you can actually enable the mixin for embedded models even :)

[ShaharHD]

@clarkbw any updates regarding the possible merge?
I'm currently hosting this npm on my private npm repo

[ebarault]

@ShaharHD, thanks for getting back,
well unless i don't get you that's exactly what i'm doing:

• first a post on father/
• then a post on father/$fatherId/passengers -> createdAt is created in embedded passenger
• then a put on father/$fatherId/passengers/$passengerId (not passing createdAt in the put)-> createdAt is refreshed to update date

did you mean something else?

[ShaharHD]

@ebarault

couple of ideas to try out:

1. Make sure Passenger model is saved in a transient datasource (datasources.json).

  "transient": {
    "name": "transient",
    "connector": "transient"
  },

and in model-config.json:

  "Passenger": {
    "dataSource": "transient",
    "public": false
  },

1. At the Trip passenger relation, add the next snippet after "property": "_passengers"

"options": {
        "validate": true,
        "persistent": true
      }

[ebarault]

@ShaharHD : all config is ok, still does not work. i'm good to pass the createdAt value when updating for now.
Do you know if there is a pull request or a tracker already for this bug you mention?
(quote: "bug on juggler which events are not triggered correctly for embedded models")

[ShaharHD]

@ebarault
loopbackio/loopback-datasource-juggler#480

[ebarault]

@ShaharHD : loopbackio/loopback-datasource-juggler#516
... pfiewww what a rabbit hole! i definitly run into issues when dealing with embeds relations for a while now. Seems the loopback team too!! Agree these should be tagged as Labs features.

[ShaharHD]

@ebarault 👍
So, I did some more testing on my end.
Embedded documents are a hassle - actually because of the createdAt validation errors I added the ability to do null for "createdAt" and avoid the injection per model (which I see you actually used to overcome this issue).

createdAt is updated on the embedded models because it doesn't have the model to reference to when referencing this.

I actually have some more modifications planned. for example:

• Try to extract the createdAt from the record Id (valid for mongodb only) - if I don't have a createdAt value.
• Have an explicit mode where I actively go and read the record for upsert operations (huge performance hit) - but might be the only way for now.

Further more, I did talk with the StrongLoop sales guy, more than once regarding this issue, that the loopback people are ignoring a few troublesome issues - without even acknowledging them.

In any case, I'm still waiting on @clarkbw for some response, but I'm guessing he's pre-occupied for now, so my projects are actually referencing my git branch.

[ebarault]

did you mean updatedAt instead of createdAt? These were actually causing me
some missing properties validations errors.
Yeah, i'm graceful you added this option to deactivate it with null, 'was
exactly the kind of tuning i was about to make.

Oh yeah you're right, i had a few occasions to comment on these kind of
issues the loopback team tend to simply throw "under the carpet"...
quote: "the feature is implemented (, period)". that's too bad.

2016-03-08 23:19 GMT+01:00 Shahar Hadas [email protected]:

So, I did some more testing on my end.
Embedded documents are a hassle - actually because of the createdAt
validation errors I added the ability to do null for "createdAt" and avoid
the injection per model (which I see you actually used to overcome this
issue).

Further more, I did talk with the StrongLoop sales guy, more than once
regarding this issue, that the loopback people are ignoring a few
troublesome issues - without even acknowledging them.

—
Reply to this email directly or view it on GitHub
#18 (comment)
.

[ShaharHD]

@ebarault yep. my bad. updatedAt - in my testing I disabled validate at the relations to bypass some problems.

[clarkbw]

Ok, I'm back and reviewing everything now. Sorry I got caught up with other things.

[clarkbw]

I created #20 to update the module to the latest build system. This is the pain of trying to use "latest" in all your devDependencies. I'll need to rebase your patch on top of that work once it is finished.

[ShaharHD]

@clarkbw you need me to do catchup on my branch?

[clarkbw]

Ok, I didn't notice this before but it appears that your changes were made to the generated files in the main directory and not the files in the es6 directory. See https://github.com/clarkbw/loopback-ds-timestamp-mixin#development You can run npm run watch while you're developing to have the base directory files auto generated.

I'll try to make your changes right now in a new branch to run the tests and will report back.

[ShaharHD]

My bad ... didn't read that... just went ahead and fixed it :)

[clarkbw]

My bad

No, thank you for this awesome work! 👍

And again, sorry I've had such crappy turn around time. 😐

[ShaharHD]

As I said before, I actually have some more modifications planned. for example:

1. Try to extract the createdAt from the record Id (only valid for mongodb probably - but still useful) - if I don't have a createdAt value.
2. Have an explicit mode where I actively go and read the record for upsert operations (huge performance hit) - but might be the only way for now.
3. Add the option to make the createdAt and updatedAt as required: false to avoid the validation errors and do it in pure code.

You did most of the work here ... I just adopted it to overcome some of LB shortcomings :)

[clarkbw]

Try to extract the createdAt from the record Id (only valid for mongodb probably - but still useful) - if I don't have a createdAt value.

Would be interesting if you can possibly grab it from the instance value without a lookup.

Have an explicit mode where I actively go and read the record for upsert operations (huge performance hit) - but might be the only way for now.

I've struggled with this because the reason you do an upsert is for performance, otherwise you could do your own select and update || create. When we take away performance the upsert becomes syntax sugar, which is possibly useful but doesn't feel like a good default. If this was a run time option developers could choose to take the perf hit as desired.

Add the option to make the createdAt and updatedAt as required: false to avoid the validation errors and do it in pure code.

So pull the validation internally instead of using the juggler? Feels like it will require lots of tests, I'll be interested to see how you do it.

[clarkbw]

Ok, I created https://github.com/clarkbw/loopback-ds-timestamp-mixin/tree/pr/18 with all of your changes in the correct es6 directory however the tests still fail. I'm looking into it but it appears that the currentInstance isn't working as intended or the tests are incorrect.

If you want to take the changes I organized and apply them to your current branch you could continue working from there. It's probably best if you update your code, rebase to master and force push. You could open a new PR with the updated code branch I made if that's easier.

I'll do a little more digging as well but something is off with the upserts, the timestamps are no longer the same. Tests like this one fail, https://github.com/clarkbw/loopback-ds-timestamp-mixin/blob/master/test/test.js#L52-L64

[ShaharHD]

Thanks,

I'll work on this on the weekend probably.

[alterx]

Hey guys, any updates on this?

[ShaharHD]

I'm still trying to clear some time to work on this. I'll probably do it this weekend.

[alterx]

Cool, just wanted to know if someone was working on this :)

[clarkbw]

I've added an option for validateUpsert in 44ab516 This option allows you to stop the mixin from forcing validation off. However it also means that upserts will fail, this is exactly how I tested it ( https://github.com/clarkbw/loopback-ds-timestamp-mixin/blob/master/test/test.js#L207-L228 )

[clarkbw]

@ShaharHD I took some of your better error reporting but struggled to understand the purpose of the rest of your code. We can talk about it more here, but for now with the option to be able to validate most of the concerns here should be resolved.