[BUG] Nested required attribute is enforced, even when optional parent is not defined

[ebrearley]

Summary:

Let's say that I have a store that sells two products, apples and boxes and I define the cart item schema like this:

Notice about schemaObject. It is an internal method which can change at any time without warning. It's use is highly discouraged. More information in the documentation.

Schema

const AppleAttributes = new Schema({
  variety: {
    type: String,
    enum: ['red', 'green'],
    required: true,
  },
  quantityKg: {
    type: Number,
    required: true,
  },
})

const BoxAttributes = new Schema({
  heightMm: {
    type: Number,
    required: true,
  },
  widthMm: {
    type: Number,
    required: true,
  },
  depthMm: {
    type: Number,
    required: true,
  },
})

const CartItem = new Schema({
  id: {
    type: String,
    hashKey: true,
    required: true,
  },
  type: {
    type: String,
    enum: ['APPLES', 'BOX'],
    required: true,
  },
  appleAttribues: {
    type: Object,
    schema: AppleAttributes.schemaObject,
  },
  boxAttributes: {
    type: Object,
    schema: BoxAttributes.schemaObject,
  },
});

The application does two things with this schema:

• appleAttribues is defined when the type is APPLES
• boxAttributes is defined when the type is BOX

Neither appleAttribues or boxAttributes are required, but they themselves have required attributes.

The problem

We cannot leave boxAttributes undefined without getting the following validation error:

ValidationError: boxAttributes.heightMm is a required property but has no value when trying to save document

same goes for leaving appleAttribues undefined.

What I think the behaviour should be

Only run required validation checks on an item that has it's parent value defined.

---

Environment:

Operating System: MacOS
Operating System Version: 10.14.6 Mojave
Node.js version (node -v): 12.14.1
NPM version: (npm -v): 6.13.4
Dynamoose version: 2.1.2

Other:

• I have read through the Dynamoose documentation before posting this issue
• I have searched through the GitHub issues (including closed issues) and pull requests to ensure this feature has not already been suggested before
• I have filled out all fields above
• I am running the latest version of Dynamoose

[fishcharlie]

@ebrearley Few things:

1. schemaObject is technically not documented. It is highly discouraged from using this since it can break or change without a major version. See the following link for more information: https://dynamoosejs.com/other/FAQ#can-i-use-an-undocumented-property-class-method-or-function-in-dynamoose
2. This will be getting better with nested Schema support (hopefully coming soon)
3. I'm planning on having support to have a better contract where you can set it up where appleAttribues is required if type is APPLE and where boxAttributes is required if type is BOX. Which would be super cool. Not currently possible today I don't think. But something I am planning.

Now on to the main topic. I thought about this a lot. I could have SWORN I considered this a lot and had a reason for everything I implemented when it comes to this. The goal was to have it so that you could have a lot of flexibility and to where anything was possible here.

This does sound like a bug to me, since it doesn't achieve those principles. There is no way to have a sub property required only if the parent exists. If you want both to be required, you can set required on both properties (parent & child). If you want the child to be required if the parent doesn't exist, you should only set required on the child. If you want the parent to be required but the child to not be required, only set required on the parent.

I'm pretty sure that summarizes my goal of what this should be. I don't think there are any other edge cases to consider here.

I will add this to the todo list, and try to reproduce it and look into it more.

[ebrearley]

1. Thanks for the heads up. Would you like me to update the initial post to merge the schemas into one, just in case others see this and decide to use the schemaObject property?
2. Great!
3. VERY cool! Very much will be using this functionality when it is available.

Yes, can confirm, this absolutely summarises everything I'd like to be able to do.

[fishcharlie]

@ebrearley

Thanks for the heads up. Would you like me to update the initial post to merge the schemas into one, just in case others see this and decide to use the schemaObject property?

Up to you. It might be useful for others. But then my comment might get confusing since it's unclear what I was referring to. Maybe adding a warning with a link to my comment about why it's a bad idea?

At this point I've done all I can to mention that it's a bad idea. If you wish to help communicate that more, that'd be great. But I've done all I can haha.

---

If you'd like you can look into creating a pull request for this. I'm really focusing on some of the single table design things currently. I'm almost done with 1 of the 5 urgent single table design features I want to do. Hoping to speed up the development on that, and hopefully get that out ASAP.

Otherwise, I'll get around to this when I can. Thanks for the report tho!

[ebrearley]

@fishcharlie

I thought I'd give knocking up a PR for you a crack and I stumbled onto something that looks a lot like it does exactly what we want it to do:

dynamoose/lib/Document.ts

Lines 371 to 394 in e70f5ca

	if (settings.required) {
	let attributesToCheck = Document.attributesWithSchema(returnObject, model);
	if (settings.required === "nested") {
	attributesToCheck = attributesToCheck.filter((attribute) => utils.object.keys(returnObject).find((key) => attribute.startsWith(key)));
	}
	await Promise.all(attributesToCheck.map(async (key) => {
	const check = async (): Promise<void> => {
	const value = utils.object.get(returnObject, key);
	await model.schema.requiredCheck(key, (value as ValueType));
	};
	const keyParts = key.split(".");
	const parentKey = keyParts.slice(0, -1).join(".");
	if (parentKey) {
	const parentValue = utils.object.get(returnObject, parentKey);
	const isParentValueUndefined = typeof parentValue === "undefined" || parentValue === null;
	if (!isParentValueUndefined) {
	await check();
	}
	} else {
	await check();
	}
	}));
	}

You weren't wrong, you've definitely given this a lot of thought! You've even implemented logic that reflects the intended behaviour.

So I turned my focus towards the schema in my application to figure our what's going on. And I've found the culprit:

A default value is always applied to a key, even when the parent isn't defined.

Take this schema:

const CartItem = new Schema({
  id: {
    type: String,
    hashKey: true,
    required: true,
  },
  type: {
    type: String,
    enum: ['APPLES', 'BOX'],
    required: true,
  },
  appleAttribues: {
    type: Object,
    schema: {
      variety: {
        type: String,
        enum: ['red', 'green'],
        required: true,
+       default: 'red',
      },
      quantityKg: {
        type: Number,
        required: true,
      },
    },
  },
  boxAttributes: {
    type: Object,
    schema: {
      heightMm: {
        type: Number,
        required: true,
+       default: 100,
      },
      widthMm: {
        type: Number,
        required: true,
+       default: 100,
      },
      depthMm: {
        type: Number,
        required: true,
+       default: 100,
      },
    }
  },
});

Nested objects are not actually undefined or null when doing the required check because they're filled with objects that have the defaults defined.

If we take the above schema and apply this input:

const input = {
  id: 'some-id',
  type: 'box',
  boxAttributes: {
    heightMm: 100,
    widthMm: 100,
    depthMm: 100,
  },
}

you get the following document ouput:

// document output
{
  id: 'some-id',
  type: 'box',
  appleAttribues: {
    variety: 'red',
  },
  boxAttributes: {
    heightMm: 100,
    widthMm: 100,
    depthMm: 100,
  },
}

---

So, the upshot!

The required check is fine, doing it's job well! 👏

Default attributes probably should only be applied when the parent key is defined. Thoughts @fishcharlie ?

[fishcharlie]

@ebrearley Without thinking about it much, yes, default should have the same behavior as required in this context.

[fishcharlie]

@ebrearley What is the status on this? Any chance you look into creating a PR?