Preloading polymorphic relationship breaks on ember-data 5.3

[leoeuclids]

Description

Preloading a polymorphic model id on a belongsTo relationship triggers a associated records were not loaded error, but works when preloading with the base model id.
The expected behavior would be that preloading a relationship, with both the base model id and polymorphic model id, would work the same way, which they don't on ember-data 5.3.

Reproduction

Simplified reproduction project

Versions

yarn list v1.22.19
├─ @ember-data/[email protected]
├─ @ember-data/[email protected]
├─ @ember-data/[email protected]
├─ @ember-data/[email protected]
├─ @ember-data/[email protected]
├─ @ember-data/[email protected]
├─ @ember-data/[email protected]
├─ @ember-data/[email protected]
├─ @ember-data/[email protected]
├─ @ember-data/[email protected]
├─ @ember-data/[email protected]
├─ @ember-data/[email protected]
├─ @ember-data/[email protected]
├─ [email protected]
└─ [email protected]

[runspired]

iirc polymorphism in 5.2 has a bunch of broken edge cases, I wouldn't necessarily consider it the definitive version of "Was it supposed to work this way". We fixed it in 5.3 and backported the fix to 4.12. That is assuming of course this is even related to those changes, I suspect its not. It's probably other improvements around identity and simplification of internal timing that then exposes you to the losing end of a race condition here.

Generally speaking, we have never explicitly supported polymorphic find in this way. That said, we did some creative things when we landed the identifier cache in the mid 3.x series to make this scenario work most-but-not-all of the time (it used to work never). This most-of-the-time approach works whenever two polymorphic identities can be detected to belong to the same identity and we have not created a record for the initial identity (it also works some of the time when we have, though thats complicated).

If you want this scenario to work all of the time, then you end up needing to implement the identity generation hooks yourself, and using the context of your app domain to ensure the polymorphic identities are merged into a single identity. There's a bunch of polymorphic scenario tests that may help steer you in the general direction of what to do if it comes to it. This said in general findRecord and preload are two APIs that will almost certainly get you into trouble with polymorphism.

On this line when you preload, this is going to cause the entity identity to be created, and it won't know that company is valid for it.

All this said, the easy way around this is probably just to not use store.findRecord in favor of using the request pattern. After configuring any handlers you need for response normalization:

store.request(findRecord('property', propertyId, {}, { resourcePath: entities/${entityId}/properties }))

If you are making this sort of request a lot you can either create your own builder (there's utils for builders too), or just wrap an existing builder to do the above if that's pretty much what you need.

[runspired]

After digging in more I can give you more insight into why what you are doing breaks in 5.3 vs 5.2.

At the point you initiate the preload request, the store has already realized the entity-1 was company-1 and merged the two identifiers. Then the preload swaps the information for the relationship back to entity-1 generating a new identifier. Basically, because the preload is treated as "cache state", you replace what past payloads told you and end up somewhere else.

This becomes a problem in 5.3 specifically because we had to remove one of the more creative things we'd done to try to enable polymorphism to mostly work even without the consuming app configuring the identity hooks:

data/packages/store/src/-private/caches/identifier-cache.ts

Lines 562 to 566 in 92f5f66

	// ensure a secondary cache entry for this id for the identifier we do keep
	keyOptions.id.set(newId, kept);
	// ensure a secondary cache entry for this id for the abandoned identifier's type we do keep
	let baseKeyOptions = getTypeIndex(this._cache.types, existingIdentifier.type);
	baseKeyOptions.id.set(newId, kept);

These lines were creating a reverse lookup so that if we merged two identifiers (as happens in your case) and later encounter the discarded identifier again (which happens due to the preload data being assigned to an already loaded record in your example) we would recognize that this was a previously merged identifier and return the original.

The trouble is this meant that we couldn't give the identity generation hook the full control it needs for the case where resources are not json values, and this hack for polymorphism was only ever going to work with json that had id specifically as top level field as opposed to say urn or primaryKey or anything else that might be equally valid.

As an aside, none of this would matter if it weren't for the fact that the route and parent route attempt to rerender with the new state you are preloading while the request is still in-flight. This is yet another reason why using requestManager here instead would be beneficial.

I'm considering whether there's some other way to bring back the utility of the reverse lookup without breaking the opaque idea of resources these APIs demand. I'm not sure there is, and I'm also not sure the utility of it given in more modernized usage this problem and many more like it wouldn't be encountered, but if it turns out to not be too hard then its probably ok to do.