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

ColumnValue introduction #12

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

ColumnValue introduction #12

wants to merge 3 commits into from

Conversation

cbschuld
Copy link
Owner

Overview

An introduction of ColumnValue which was initially conceived by @MarkHerhold

Adds

  • The ColumnValue class which stores the results from the RDS and provides easy typed results

Updates

  • bump in tests to deal with the breaking change.

@cbschuld
Copy link
Owner Author

@MarkHerhold bummer, I have bumped into that socket hang up error as well

@cbschuld
Copy link
Owner Author

Fixes #6 - @MarkHerhold your idea behind ColumnValue I believe cements this usability issue. Marking closed on this PR.

@cbschuld
Copy link
Owner Author

Fixes #7 - ColumnValue provides this and supports enum as denoted.


get buffer(): Buffer | null {
if (this.isNull) return null;
return Buffer.from((this.field.blobValue || '').toString());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why should do we call .toString() on a value that I'd presume to be a string?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I think this logic is wrong for binary type.

The AWS docs say:

blobValue - A value of BLOB data type. Type: Base64-encoded binary data object

So I think we would need to do Buffer.from(this.field.blobValue, 'base64'). I don't see a test that explicitly tests this logic and I haven't verified my assumption with the real RDS Data API.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disregard my prior comment - that's the API documentation, not the JS SDK docs.

It does look like blobValue is already a Buffer type so this value manipulation would be unnecessary, both in this PR and on master.

image

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MarkHerhold - looks like the blobValue can return as Buffer|Uint8Array|Blob|string possible answer I am committing now:

return Buffer.isBuffer(this.field.blobValue) ? this.field.blobValue : Buffer.from(this.field.blobValue as Uint8Array);

@cbschuld
Copy link
Owner Author

TODO: I will update the test dB and add a binary column and create a test

@lbecker34
Copy link

@cbschuld just taking a look at the new code and there will still be a problem with Decimal columns saving and reading.

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

Successfully merging this pull request may close these issues.

None yet

3 participants