-
Notifications
You must be signed in to change notification settings - Fork 81
feat: index projection & index scanning #509
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
base: master
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for electrodb-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Hey @anatolzak 👋 Firstly, wow, just wow. This is easily the most extensive feature PR this project has ever received (excluding the time someone submitted a PR for electrodb.dev). I will need some time to review this, but thank you for including both code and type tests with your PR. Two things that may come up in review (note: I have not read through this PR yet):
|
|
@tywalch, first, thank you for the kind words! :) Regarding the two points you mentioned:
Looking forward to your feedback! |
|
I'm starting to review this today. You'll find I am very cautious around new changes and I won't be rushing to get this merged, but at offset I am very impressed with all the considerations I am seeing you make in your tests 💪 |
|
@anatolzak I made some progress on this today and two things shook out:
|
|
|
@tywalch I finished adding the type narrowing for service collections based on the DynamoDB index projection type, along with many type tests. a few notes:
|
Awesome! I'll take a look tomorrow to familiarize myself with everything 💪
Yes, likely that latter (perform the attribute check based on all the valid attributes of the entries in the collection. I'd like to take a look at this tomorrow as well. Services delegate queries to a member entity and then use execution options to change the behavior of the Entity. It's all very hacky and I want to see if I can stumble into a fast solution there.
This makes sense; I will be the first to say |
|
|
||
| ### Entity Identifier Requirements | ||
|
|
||
| When using `INCLUDE` projections, you **must** include ElectroDB's internal entity identifier attributes (`__edb_e__` and `__edb_v__`) in your projection. These attributes are essential for ElectroDB's multi-entity guardrail checks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can own this, but we'll want to create an abstraction for this somehow. I'm still soul searching on the best approach (and open to ideas), but I typically try to make sure things "just work" (™) for greenfield devs.
For a few reasons, my instinct is to disable ownership checks by default and use opt-in for developers who include identifiers in their GSI projection attributes.
The most significant exposure here is for Services that use a collection on an INCLUDE GSI that also uses the template syntax. The move here might be to recognize the risky scenario and branch into an additional validation at Service instantiation; namely, more stringent checks to ensure isolation in key formatting.
These are just musings right now, though I'm interested in your thoughts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a few reasons, my instinct is to disable ownership checks by default and use opt-in for developers who include identifiers in their GSI projection attributes.
The most significant exposure here is for Services that use a collection on an
INCLUDEGSI that also uses thetemplatesyntax. The move here might be to recognize the risky scenario and branch into an additional validation at Service instantiation; namely, more stringent checks to ensure isolation in key formatting.
I don't think I have a better solution than what you proposed. I think it's fair to not do ownership checks if the user doesn't include the identifiers attributes in the GSI. We could maybe log a warning if the identifiers are not included and provide an option to suppress the warning
An update on this front: I have a working local branch with these changes. I will be working on additional tests for it tomorrow and hope to have it ready for you to merge with your PR. |
That's awesome! Thanks so much for doing this! 🙏 |
|
hey @tywalch 👋 let me know if there is anything I can do to help! |
9b1f191 to
c6a34e0
Compare
Hey @tywalch 🙂 Just checking in to see how things are going with the review and the additional changes you mentioned (collection attribute checks, ownership checks, etc.). Is there anything I can do to help move this PR forward? Thanks! |
|
Hey @tywalch! I went ahead and updated the PR to make it merge-ready. Previously, there were two outstanding issues:
Here’s how I addressed them:
With these changes, I think the PR is in a good place to merge. Let me know what you think, or if there’s anything else I can do to help. Thanks! 🙏 |
effe682 to
c3fbb44
Compare
|
Hey @tywalch! Just a quick nudge to take a look at this PR when you get a chance. I think it should be good to merge. Thanks so much! |
c3fbb44 to
38ba77a
Compare
|
Hey @tywalch! Just checking in on this PR. Happy to adjust anything that doesn’t look right or make any changes you’d like. Let me know if there’s anything I can improve. Thank you! |
|
I just came across this PR, and it immediately caught my attention. We recently ran into a very similar issue at my company that stemmed from the lack of type safety when working with index projections. It ended up being more than a theoretical concern, we actually hit real bugs because the types didn’t accurately reflect what was being projected. To unblock ourselves, I implemented a somewhat hacky workaround in our codebase. While it solved the immediate problem, it never felt like the right long-term solution, and I was really hoping this gap would eventually be addressed directly in the ORM itself. Seeing this PR feels like a step in exactly that direction, and I’d genuinely love to see it get merged. |
|
@anatolzak I carved out some time to dig back into this and revisited the identifier flow. I noticed that validation would prevent users from adding their identifier fields, so I put together a branch for you merge into your PR: https://github.com/tywalch/electrodb/tree/feature/index-projection-enables-projected-identifiers |
5a715d3 to
547907a
Compare
|
@tywalch thank you for taking the time to revisit this PR! I have merged your branch. Please let me know if there is anything else that needs to be done. and again, I really appreciate you taking the time to review a PR that is anything but trivial 😅 |
@anatolzak I have another round of updates, same feature branch: |
- Adds and refactors changes to use `projection` property instead of extending use of `project`. The name `project` is not as self-evident as `projection`. - Deprecates `project` property. This field was not associated with any functionality prior to this feature. - Adds entity and service validation for `projection` values
…tion until runtime support is available
- Allows entity identifier fields to be included in projection array - Extends ignoreOwnership logic to consider the presence of identifiers - Exposes default identifier field names via enum style object and string array - Adds additional tests to cover new functionality.
…#517) * fix: missing ProjectionExpression on batchGet with attributes * add tests * changelog * increment version * Reduce ElectroDB pre/post-processing overhead before/after DynamoDB requests by up to 85% (tywalch#515) * reduce pre/post processing overhead * changelog * increment version * Fix missing ProjectionExpression on scan with attributes (tywalch#511) * fix: missing ProjectionExpression on scan with attributes * add tests * add changelog * 3.4.5 * Update services.mdx (tywalch#513) * typo (tywalch#492) * typo fix (tywalch#454) * Update attributes.mdx (tywalch#422) Fix example * fix: broken link in error (missing-composite-attributes) (tywalch#409) --------- Co-authored-by: aspida-will <will.krakow@aspida.com> Co-authored-by: Chris <chris.zirkel@gmail.com> Co-authored-by: Patrick McDavid <fusionjunky@gmail.com> Co-authored-by: iOSonntag <dev@iosonntag.com> Co-authored-by: Tyler W. Walch <tywalch@gmail.com>
- Enables passing attributes to a collection query
cf774a0 to
bca54a5
Compare
@tywalch I merged your changes. The tests were failing due to this code key[sk.field].startsWith(sk.prefix) //&&
(!sk.postfix || key[sk.field].endsWith(sk.postfix))I fixed it in this this commit, let me know if it makes sense Also the tests are now failing for the new feature "ProjectionExpression in batchGet" where previously the tests expected Electro to include the PK and SK in the projection expression, and now they are no longer included in the projection expression. Just making sure this was intentional on your end |
|
That's no good, I wonder if I didn't actually push my latest changes. I'm still recovering from being sick recent, I'll take a look to see what went wrong 😓 |
src/entity.js
Outdated
| typeofSkProvided === "string" && | ||
| key[sk.field].startsWith(sk.prefix) //&& | ||
| (!sk.postfix || key[sk.field].endsWith(sk.postfix)) | ||
| key[sk.field].startsWith(sk.prefix) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anatolzak, went a little further with this in my feature branch and pushed a commit ~10s ago. Could you pull that in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tywalch no problem, I pulled your changes
My previous commit was incomplete. Fixed conditional logic and improves readability.
Closes #508, #507
Currently, users who want to use DynamoDB indexes with the INCLUDE projection type do not receive any type safety from ElectroDB.
These indexes are highly beneficial for access patterns that require multiple optional filters, which a standard index cannot accommodate. By creating an index with only the attributes needed for the multi-filter access pattern, you can achieve much faster queries while scanning less data, resulting in lower costs.
It is also sometimes useful to scan a specific index when it is sparse and you are only interested in the items present in that index.
At present, ElectroDB does not offer a built-in type-safe method to scan an index.
This PR introduces two new features:
Here is an example of how to define projected attributes.
Here is an example of how to scan a specific index.
The PR primarily includes modifications at the type level to enhance type safety. Here is the type functionality:
attributesargument in the go method allows you to specify only the projected attributes.attributesparameter while scanning or querying an index with limited projected attributes.Implementation Overview
I introduced a new generic type
Pto theEntityandSchemato capture the type of the projected attributes. To ensure backward compatibility, the generic type has a default value ofstring, and the new generic is added as the last one (afterS).This change allows users to migrate to the new version of the library without encountering type errors when using the Schema or Entity types.
To ensure that the output type of queries and index scans with limited projected attributes contains only the projected attributes, we forward the "access pattern" key to the
QueryBranchesandGoQueryTerminalOptionsetc. This allows us to implement conditional types for achieving the fine-grained type safety described above.Notes
I added documentation, numerous tests, and type tests to validate all this functionality.