Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sparse index is not dropped by updates that don't include index members #479

Open
dabrowne opened this issue Feb 21, 2025 · 1 comment
Open

Comments

@dabrowne
Copy link

dabrowne commented Feb 21, 2025

Firstly, thanks a bunch for this great library. I've worked with DynamoDB for a few years now, but it's only using ElectroDB that I feel like its full capabilities can be used without getting bogged down in boilerplate.

Describe the bug
The condition predicate for a sparse index is not evaluated for update calls (update(), patch(), or upsert()) that don't include all members of the index.

This means that if the condition is independent of the composite index members, then it is quite easy to write an update call that you would expect to result in the index being dropped, but does not. This is best described by the example in this playground link. In summary:

  • The byName index is sparse on the condition status !== "deleted".
  • When an update call sets status: "deleted", this condition is not evaluated, and the index is not dropped.
  • Bringing the byName index "into scope" by providing its composite fields to the .composite() call (commented out in the playground) causes the condition to be evaluated and the index to be dropped as expected.

Expected behavior
The index should be dropped when an update call sets status: "deleted".

I think that there is probably no practical way to change this behaviour, as it would require evaluating all condition predicates on every update. I expect that many/most condition implementations assume that the index members values will be available, so evaluation of these conditions in a different context would break a lot of existing code.

Short of fixing/changing the behaviour, is there a way to improve the documentation to help developers form a better mental model about how this works? Perhaps something along the lines of:

The condition callback is only evaluated for updates that either involve all index members, or where the index members are provided using composite().

I also found this misleading in the documentation for condition, which implies that the condition is evaluated for every mutation:

A function that accepts all attributes provided every mutation method and returns a boolean value.

Additional context
Related: #366

@dabrowne
Copy link
Author

dabrowne commented Feb 21, 2025

Bringing in some relevant discussion from #366 that I glossed over on first read. It looks like this limitation has already been discussed and understood, although I think that the caveat 1 mentioned here isn't well captured in the current documentation.

Aside from fixing this at the docs, I think there is also a backwards-compatible way to fully resolve this by introduction of a new conditionDependencies array (pending a better name), as described by @sam3d:

Add a list of dependent attributes to condition (like with watch on attributes) so we know when to re-compute the presence of the index. This would retain the current flexibility and fix index behaviour on mutations, with some added complexity for the entity index definition

Here's how I picture this working:

  1. This is an optional field, defaulting to []. If you want to write a condition that is dependent on a particular non index member being present, you can choose to include its name in this array. By doing this you are guaranteed that it will be present when your condition is evaluated.
  2. When validating update operations, ElectroDB will require that either all or none of the following attributes for sparse indexes are present in the query (via either set() or composite()):
    [...pk.composite, ...sk.composite, ...conditionDependencies]
  3. If some, but not all of those fields are present, a runtime error is thrown.
  4. If none of the fields are present, the condition is not evaluated and the composite index is not changed by the query.
  5. If all of the fields are present, then the condition will be evaluated and the index will be updated or dropped based on the outcome.

This is really only a small extension of the current behaviour and is done so in a backwards-compatible way: The check that was introduced to fix caveat 2 raised here would still apply in the default case of conditionDependencies: [], however if a developer wants to add dependencies to their condition beyond the composite index members, they can use conditionDependencies to extend that check to include additional fields. This gives a nice guarantee that you won't be able to make the mistake in my original playground link without encountering an error.

Here's an updated playground link showing how I picture a developer would use this.

@dabrowne dabrowne changed the title Sparse index is not dropped for updates that don't include index members Sparse index is not dropped by updates that don't include index members Feb 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant