tywalch / electrodb

A DynamoDB library to ease the use of modeling complex hierarchical relationships and implementing a Single Table Design while keeping your query code readable.
MIT License
1.02k stars 64 forks source link

Empty array as default for "set" type shouldn't pass schema validation #427

Open raryanpur opened 2 months ago

raryanpur commented 2 months ago

Describe the bug An empty array as the default value for the "set" type shouldn't pass schema validation, since DynamoDB doesn't allow empty sets ("Sets" section on this page: https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/HowItWorks.NamingRulesDataTypes.html).

Having a little trouble testing but I think the DocumentClient will throw an error in this case. It's possible to pass options.convertEmptyValues=true to the DocumentClient, but since that's not the default could be a bit confusing if the ElectroDB schema passes validation but the DocumentClient throws an error.

ElectroDB Version 2.14.3

ElectroDB Playground Link (if possible) Use the ElectroDB Playground to recreate your issue and supply the link here to help with troubleshooting.

Entity/Service Definitions Include your entity model (or a model that sufficiently recreates your issue) to help troubleshoot.

{
    model: {
        entity: "my_entity",
        service: "my_service",
        version: "my_version"
    },
    attributes: {
        prop1: {
            type: "string"
        },
        prop2: {
            type: "string"
        },
        tags: {
            type: "set",
            items: "string",
            default: []
      },
    },
    indexes: {
        record: {
            pk: {
                field: "pk",
                composite: ["prop1"],
            },
            sk: {
                field: "sk",
                composite: ["prop2"],
            },
        }
    }
}

Expected behavior

Schema shouldn't pass validation if the default value of a set is [].

tywalch commented 2 months ago

Hi @raryanpur 👋

Thanks for posting! The philosophy of validation in the library has generally been to focus on verifying Electro specific conventions and then, for everything else, get out of the user's the way. The main reason for this is because limits, defaults, and validations in DynamoDB are all subject to change. To date, this boundary has been very helpful in maintaining the library's stability.

raryanpur commented 2 months ago

@tywalch thanks for the reply, and that definitely makes sense. Agree that's the right approach.

I think it's helpful for the schema validation in Electro to generally track the modeling boundaries of Dynamo, since the library is tightly coupled to Dynamo. Specific Dynamo limitations on values of data types (i.e. in this case, not allowing empty sets) feels close enough to schema modeling that it could be considered an Electro convention as well (and enforced by Electro), but there's definitely some grey area.

To your point of not including this as an Electro validation - since the user can set a flag on the DocumentClient to "handle" empty sets, not having Electro validate provides for more flexibility. I'm not sure if there would be any special considerations for setting this flag on the DocumentClient, but at least the user has the optionality as things are now.

Could see it making sense either way.