[BUGFIX identifiers] Address issue with polymorphic findRecord

[krasnoukhov]

Hello! I found this issue while updating our app from 3.12 to 3.16. Previously it was possible to use findRecord to retrieve a specific instance of polymorphic model through the "base" model endpoint. This is useful when only the id is known but not the specific type. There was a prior fix in #6644 but the difference in this case is that there's no cache (through the dealership) yet.

So far I'm only adding a failing test which runs into this assertion:

data/packages/store/addon/-private/identifiers/cache.ts

Lines 519 to 524 in 78162a0

	// TODO consider just ignoring here to allow flexible polymorphic support
	if (type && type !== identifier.type) {
	throw new Error(
	`The 'type' for a RecordIdentifier cannot be updated once it has been set. Attempted to set type for '${wrapper}' to '${type}'.`
	);
	}

My next thought was to remove that assertion (since TODO says that it may be problematic), and I tried that, but I have a feeling that'd not be a complete fix since foundFerrari.constructor.modelName in the test resolves to car and not ferrari. I checked this on 3.12 and I believe it should be ferrari. UPD: visible in this CI run

I tried digging into the source but I'm a little bit lost really so any feedback would be appreciated. I hope we can address this and backport into 3.16.

Thank you!

[krasnoukhov]

Ok, I narrowed it down to the fact that lid is assigned when findRecord is performed. Seems like not doing that when type does not match would be a safe bet and should only apply in polymorphic context, so a new identifier will be created down the road.

Let me know if I should add some more tests. I'd be happy to consider alternative approaches as well.

[snewcomer]

Very nice PR! Looks like your tests cover when REQUEST_SERVICE is on and off.

[krasnoukhov]

Thanks for the approve. Yeah, that integration test is very high level. Sounds like this is going to be merged? I can create a PR into lts-3-16, I already have these changes backported. Maybe lts-3-20 as well? Let me know

[hjdivad]

@igorT @runspired do either of you see any issues with the implementation? Broadly this LGTM

[igorT]

@krasnoukhov thanks a bunch for looking into this, great job dealing with the complex codebase without prior experience.

I have run into this issue previously, and added a fix for the case handling it here:

data/packages/store/addon/-private/identifiers/cache.ts

Line 558 in ff4f911

// If the lids are the same, and ids are the same, but types are different we should trigger a merge of the identifiers

I am wondering why this doesn't trigger the merge, it should conceptually be handled by the merge function. @snewcomer do you mind taking a look with @krasnoukhov to see why it doesn't get triggered

[krasnoukhov]

Thanks @igorT. I stumbled upon that code as well. I tried adding debugger to both detectMerge and updateRecordIdentifier and these are not even called within that test scenario that I've added. Not really sure whether that's expected, any pointers will be appreciated...

[igorT]

@snewcomer lets figure out why detectMerge isn't getting called

[snewcomer]

I took a look at the cache and here are my thoughts.

Seems to me this is the right solution. detectMerge only deals with existing cache items, thus no existing identifier to "merge" as shown in this PR's test case. Moreover, this solution in this PR is able to get the correct lid much earlier in the process (at the outset of find) before propagating through the many layers.

data in detectMerge:

An alternative would be in detectMerge to makeStableRecordIdentifier from the data if we detect a mismatch of lid or type. This new identifier would then be passed to _mergeRecordIdentifiers.

[snewcomer]

To clarify further, without these changes, the identifier created further upstream looks like this.

As a result, the IdentifierCache by types cache has an entry of car. Because this is actually a ferrari, there is no cache entry and thus our logic dictates we should not "merge identifiers".

However, with these changes, not setting the lid when we go to _push the data into the store will generate a new internalModel for us with the right lid/type of ferrari.

If we set the lid as we do without this PR, then we have what we need and thus don't end up creating an entry in the IdentifierCache by type for the new ferrari. The cache just looks like this...

Lastly, simply commenting and not making any conclusions - I noticed one other thing. The internalModel is in a state of loading here without these changes. With these changes the rootState is root.empty. I'm not sure why exactly the state is such.

[snewcomer]

👋 @krasnoukhov Igor presented another solution. Although your solution is a great idea and perhaps cleaner (is getting the lid correct further upstream better? I would assume so), the 👇 aligns us closer with how the current system works. Notice the surrounding code deals with mismatches as well. Let us know your thoughts (still one thing to deal with on a partner test but close to ready)!!

https://github.com/emberjs/data/pull/7363/files

[krasnoukhov]

Thanks @igorT and @snewcomer! Huge 🤦 moment for me since I assumed updateRecordIdentifier isn't called but that was totally because of the changes I already made to address the issue.

I think this alternative solution is definitely much more cleaner in a sense that identifier update handling stays close together. Nice work and I appreciate your help with digging into this

[krasnoukhov]

Closing in favor of #7363