Properly warn when an updated record has a null id

[runspired]

WIP, I believe this fixes but I'm running our test suite against the sha now. Will update when complete.

[bmac]

@runspired do you have time to add a test for this usecase?

[fivetanley]

hard to tell from the diff, but how does moving the assertions fix the error?

[runspired]

@fivetanley the original location was not guarded. Any of three different request types could be returning data to this function with a null id. For all three request types, there are in fact situations in which it was previously acceptable to have a null ID.

• One of the most basic checks is the if (data) {}, since it is allowable to not return any data at all.
• We should still assert for the situation in which you had not set an ID and you didn't send any data at all on the way back either, I'll add that check in.
• if you do have data, and that data has a null ID, it should warn and not overwrite an existing ID to be null (I'm realizing my check on this is inadequate re-reading my fix this morning)

In addition to the above fixes, there's additional reports (both at LI and on Slack) of records returning IDs and still erroring out on this assertion. This makes me suspect that either something was previously broken here that we only now see, or that something else broke between 2.7 and 2.8 which this assertion happens to catch.

P.S. do we have a good testing pattern for "this should warn"? (seeing as warn is not "throw")

[fivetanley]

It would be great to have some more examples of what payloads are triggering these kind of errors in people's apps. We just want to be very careful introducing any semantics with IDs.

[fivetanley]

P.S. do we have a good testing pattern for "this should warn"? (seeing as warn is not "throw")

There is expectWarning:

data/tests/integration/inverse-test.js

Line 132 in 3e32efe

assert.expectWarning(function() {

[fivetanley]

I'm also in favor of reverting #4541 until we can work out all the semantics here.

[fivetanley]

Oops, meant #4408

[runspired]

After digging into this more, I think I'm narrowing in on having asserted the correct things and found the edge cases we were missing.

One thing to note that in investigating LI's failed tests, I discovered we were often using createRecord().save() as an "API action" replacement, hence no ID being generated client side or returned server side. The model was just there to facilitate this. Surfacing a better pattern for this more easily would be nice, I'm implementing some custom adapter semantics for this on our end.

[runspired]

Closing in favor of a different PR