Skip to content
This repository has been archived by the owner on Mar 20, 2022. It is now read-only.

fix(schemas): don't use schema to attribute mapping on singular array schemas #387

Merged
merged 2 commits into from
Jan 23, 2020

Conversation

fpipita
Copy link
Contributor

@fpipita fpipita commented Jun 20, 2019

Problem

Denormalization does not seem to produce the correct result when a singular Array schema owns an Object schema whose instances own an id property.

For example:

const cats = new schema.Entity('cats');
// catRecord instances own an `id` property
const catRecord = new schema.Object({
   cat: cats
});
const catList = new schema.Array(catRecord);

The problem is in the the denormalizeValue method of the Polymorphic class.

When the value to be unvisited owns a property named id, then this method uses the value of the id property as the input value for the call to unvisit in place of the value itself, which is not the case on singular Array schema, where the value itself should always be used instead.

Solution

I've added a guard which prevents the value of the id property to become the input to unvisit when the current schema is a singular schema.

TODO

  • [ x] Add & update tests
  • [ x] Ensure CI is passing (lint, tests, flow)
  • Update relevant documentation

@coveralls
Copy link

coveralls commented Jun 20, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling c593cc3 on fpipita:master into b5f570a on paularmstrong:master.

@@ -45,7 +45,7 @@ export default class PolymorphicSchema {
if (!this.isSingleSchema && !schemaKey) {
return value;
}
const id = isImmutable(value) ? value.get('id') : value.id;
const id = this.isSingleSchema ? void 0 : isImmutable(value) ? value.get('id') : value.id;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid confusion for others, since void 0 isn't common, can we use a canonical undefined here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I've just updated the patch and rebased it to upstream/master.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants