Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Support binary type natively #822
Support binary type natively #822
Changes from all commits
abe75c5
7effcca
6e68419
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 suppose that now new values of a binary field will be stored in DynamoDB as
binary
but all existing values are stored asstring
. It might be a bit surprising for end users.The common approach is to step-by-step migrate users from the current behaviour to the new one. We can introduce a new field's option to either keep old behaviour (by default, so there is no a breaking change in a minor release) or to switch to the new one (for new fields or new tables or in new projects).
Example is a
boolean
type that wasn't supported by DynamoDB initially so Dynamoid'sboolean
fields were stored asf
andt
. Later DynamoDB added support of the boolean type and a new optionstore_as_native_boolean
was added:Later in the next major release such migration options can be removed and we can propose users to custom types to keep old storage formats.
So I propose to add a new field's option
store_as_native_binary
that is by default false. And check it at casting, dumping and undumping.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 agree. It could cause issues for integrators even though they might not have noticed that the binary values aren't stored with binary type in DynamoDB.
I've added the
store_as_native_binary
option that should be a safe work-around, but there's still a risk with this change to not get noticed with the new major release since there will not be a breaking code change.This concerns me along with the lack of alignment with DynamoDB's single table design which is why I've started exploring
Aws::DynamoDB::Client
directly for my project.Let me know if you want me to do any other changes to this PR, or I'll leave it to you for how you want to get it out.
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.
The only way to prevent breaking changes being unnoticed that I see is to add deprecation warnings in the last minor release before the major one.
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.
Could also require the
store_as_native_binary
option be set, otherwise raise an error.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.
Regarding the single-table design - it seems to me it's out of scope of Dynamoid. Or at least of the initial scope to implement the Active Record pattern.
I am not opposed to including the new functionality into the gem related to this "pattern". But I haven't found any common and widely accepted "interface" or API. So I postponed this.
There is an issue #568 but all I heard there were either some generic thoughts or requesting somebody's project specific features. So there was nothing I can work with.