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.01k stars 64 forks source link

Runtime verification for conditional composite index fails with undefined #433

Open Talljoe opened 2 weeks ago

Talljoe commented 2 weeks ago

Describe the bug With the change in 2.14, I've now hit an issue with a conditional index that I have. Maybe there's a way I can structure it differently?

In short, I have an optional field relatedEntryId and a composite index that includes it. The index also defines a condition to only index the record if relatedEntryId is specified. Even though I specify the field in create and/or patch, I get an error that it is not specified because it is undefined. This worked before 2.14, and this issue is preventing me from upgrading (coincidentally, I'm hitting #366).

ElectroDB Version 2.14.3

ElectroDB Playground Link https://electrodb.fun/#code/PQKgBAsg9gJgpgGzARwK5wE4Es4GcA0YuccYGeqCALgUQBYCG5YA7llXWAGbZwB2MXGBDAAUKKwBbAA5QMVMAG8wAUT5V2AT0IBlTADcsAY1IBfbhiiSwAIkRwjVSzABGNgNzijUPrgVUGFwRSAF5bTShUDAB9AKC4aL4GSTgPcVFvXwV+JxwhML44FlV1LQAKUTAlSqqwSVhEAC5q2tqcrWabANwAa1wbfBrW-UxcLB9OgEYBodriDEMTTu6ehmlpG1nTQdaGKlyXVCo8ZsVZqqwYU-Paqk1pOE6-bD4AcxnW1vI0LHIrsCc6Bu2xur0sqGk10+t3uj1szywbw+0LIcB+f2agLgwJ2n3ICD2cBgaicmgAkv8ziiAbCnrkkbjod9UL8ic0uAwEMRGbUQdCjORCTAAIJUKEou4PTp8VCSFyYZHQ+AcyhisBlACUYBCAD4wAARQkAOj4UBYmp5rWAwDARgYfFNCnldVgWC4OBgYAYXGOGFtguOMBuVUFMAA8nwEJpMRggdC+Z8ITAhaLxdDJXCbDK5QrLbVraw9kZOFw5LbGG88ACoF6+JovftsIdjsHC1Ri50QIrPgXiAojFFyOoAVI8AEZKw6PwwEmha2+81Ndq9YbjiazRbW6GI1GY3HPqYtjzEfAAB4nFqfaTYSRMaOX6HSHpplHuxD-GxP7v8qyyMbHZoAG0bEuGwAF1WwTaFehfaE3wQD9em-HsbQFOBCVtX8oH-UgejgTQhFLP1pCYDROSIOQFDw+s0EwPJWyqbwZGw9g4UAiDqUPeM83xIVYNqE84FPTpXjGaY80YnwYHYcY+EXOAtV1MAAEJlLgI1eMDEkMHJGAJLAJ9+NaeCP1ErBJi-fTJOYnCgJsMFIg2DiUSgz4YIfV8cAQkSxKQqywALNCMKYv9WLAajCLLEj5CwcjcEo8L8JQdBeFwBjMJs1i7M0oltN0gZbCCwNRXAyCcSPGoE2UOJgjAQ8NU8UQclSo0irgMoqTAS4pgAJgAZmRByIU6P5kRy4l1B0ilmlQAQ4HdQog1MDUjVeKBNUa5q8lagN2s67rbAAFgAVgANkG8FIVsUbcXGvLptsSZ+s2ZbVvWhqgA

Entity/Service Definitions

const entries = new Entity(
  {
    model: {
      entity: "tasks",
      version: "1",
      service: "taskapp"
    },
    attributes: {
      id: {
        type: "string",
        required: true
      },
      group: {
        type: "string",
        required: true
      },
      relatedEntryId: {
        type: "string",
        required: false,
      },
      createdAt: {
        type: "number",
        default: () => Date.now(),
        // cannot be modified after created
        readOnly: true
      },
      updatedAt: {
        type: "number",
        // watch for changes to any attribute
        watch: "*",
        // set current timestamp when updated
        set: () => Date.now(),
        readOnly: true
      }
    },
    indexes: {
      primary: {
        pk: {
          field: "pk",
          composite: ["id"]
        },
        sk: {
          field: "sk",
          // create composite keys for partial sort key queries
          composite: []
        }
      },
      related: {
        index: "gsi1",
        condition: (e) => !!e.relatedEntryId,
        pk: {
          field: "gsi1pk",
          composite: ["group"]
        },
        sk: {
          field: "gsi1sk",
          // create composite keys for partial sort key queries
          composite: ["relatedEntryId", "createdAt"]
        }
      },
    },
  },
  { table }
);

Expected behavior Explicitly specifying undefined should work to satisfy the runtime check. Alternatively, if this is not possible, a compile-time check that prevents optional fields from being used as composite keys.

Errors

Incomplete composite attributes provided for index gsi1. Write operations that include composite attributes, for indexes with a condition callback defined, must always provide values for every index composite. This is to ensure consistency between index values and attribute values. Missing composite attributes identified: "relatedEntryId"

Additional context I can likely work around this by making custom Option/Maybe type, but it's not as clean.

tywalch commented 2 weeks ago

Hi @Talljoe 👋

You're right about hitting the case described in https://github.com/tywalch/electrodb/issues/366. You could take several directions here, but here's some background about what you see. There is a runtime validation for indexes with a condition callback that all composite attributes must be updated together, but that's only part of the issue in this case. The other challenge is that relatedEntryId (the first composite slot) is undefined, but createdAt (the second composite slot) is defined. This scenario prevents ElectroDB from being able to format the sort key value with the values provided (e.g., $tasks_1#relatedentryid_123#createdat_1728237814343). Electro uses some runtime constraints to prevent keys from getting out of sync with their underlying attributes because the circumstances of the operation itself and the side effects of user-defined attribute callbacks are relevant to evaluating the constraint. Ultimately, there is a lot that Electro can't assume or know, so it optimizes to protect data integrity.

The simplest solution here is to provide some value for relatedEntryId, like an empty string or a constant that is out of band for your type of id. This will satisfy the index constraints. Checkout this playground to see how I used a get callback and default value on relatedEntryId to accomplish this without requiring additional work to your application code.

I hope this helps! If you'd like, I can discuss the constraints in more detail or offer additional approaches, just let me know 👍

Talljoe commented 2 days ago

Thank you for the writeup. I had added the condition because of the issue with null slots of composite keys and I understand the reasoning behind 366. The link to the playground doesn't work for me, but I can guess at the gist of it.

I do believe that supplying undefined explicitly in the query should be enough to satisfy the runtime constraint. I think it would be possible to notice that a value was supplied to the update/patch query, even if it is undefined, such as these two examples. I tried to put together a pull request by using Object.hasOwn or similar but wasn't completely successful. I was able to get the composite side to ~work~ pass tests but the set side is filtering out the undefineds at some point that I couldn't quite determine. And who knows what else I might have broken. :)

Alternatively, perhaps ElectroDB could use a Symbol here to indicate "value undefined is supplied." I admit that the DX ergonomics on this wouldn't be great as this is kind of the only place it would be used, but it would be less work than needing to build out a version of it using set/get and magic values.

If it truly isn't possible to use a nullable attribute in a composite key then it's probably worth flagging that at "compile time" by stripping attributes with required: false from the candidates allowed in the composite section.

Cheers and thanks again for such great support.