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

Initialize data to buffer instead of string for non-strings #3545

Merged
merged 2 commits into from Nov 21, 2019

Conversation

@MaliaGuerrero
Copy link
Contributor

MaliaGuerrero commented Nov 19, 2019

A variable to hold the Stream data is initialized to a string. In the case that the type of the stream is not "string", data is concat'ed to a Buffer, which fails. This changes initializes data to a Buffer in case that it is not a string.

@MaliaGuerrero

This comment has been minimized.

Copy link
Contributor Author

MaliaGuerrero commented Nov 19, 2019

I'd love some help on running tests for this. I can run npm test but there's no DB running so things fail. I looked around for a test DB but couldn't find it.

@elhigu

This comment has been minimized.

Copy link
Member

elhigu commented Nov 19, 2019

https://github.com/knex/knex/blob/master/CONTRIBUTING.md#the-easy-way

There is docker-compose file for starting up test databases.

Malia Guerrero
@MaliaGuerrero MaliaGuerrero force-pushed the MaliaGuerrero:fix-clob-type branch from f4e7ded to 1d62cd3 Nov 21, 2019
@@ -342,6 +342,9 @@ Client_Oracledb.prototype._query = function(connection, obj) {
function readStream(stream, type) {
return new Promise((resolve, reject) => {
let data = '';
if (type !== 'string') {

This comment has been minimized.

Copy link
@kibertoad

kibertoad Nov 21, 2019

Collaborator

why not only initialize data once, using ternary operator? (let data = type === 'string'? '' : Buffer(0) )

This comment has been minimized.

Copy link
@MaliaGuerrero

MaliaGuerrero Nov 21, 2019

Author Contributor

I'm trying to follow the way it was before: c9e3057#diff-a86ace7e0baa34caa6f981181dc544e2

I'll see if ternary operators are common and implement if so.

This comment has been minimized.

Copy link
@kibertoad

kibertoad Nov 21, 2019

Collaborator

Ternary operators are pretty idiomatic in JS. And knex old code def not a good example of how to write code 😂

This comment has been minimized.

Copy link
@MaliaGuerrero

MaliaGuerrero Nov 21, 2019

Author Contributor

Makes sense. I'm a stickler for code style, but I get what you are saying. :)

This comment has been minimized.

Copy link
@MaliaGuerrero

MaliaGuerrero Nov 21, 2019

Author Contributor

Fixed! Let me know if there's anything else!

@@ -111,6 +111,19 @@ describe('OracleDb parameters', function() {
});
});

it('on blob', function() {
return knexClient

This comment has been minimized.

Copy link
@kibertoad

kibertoad Nov 21, 2019

Collaborator

can you use async/await instead?

This comment has been minimized.

Copy link
@MaliaGuerrero

MaliaGuerrero Nov 21, 2019

Author Contributor

This is the style of this entire file. Is there a push to get all the files to use async away? And also, are tests meant to be used on older versions?

This comment has been minimized.

Copy link
@kibertoad

kibertoad Nov 21, 2019

Collaborator

we only support Node 8+, and will convert code eventually. new one is preferred in newer syntax.

@@ -111,6 +111,19 @@ describe('OracleDb parameters', function() {
});
});

it('on blob', function() {

This comment has been minimized.

Copy link
@kibertoad

kibertoad Nov 21, 2019

Collaborator

can you make it an arrow function?

This comment has been minimized.

Copy link
@MaliaGuerrero

MaliaGuerrero Nov 21, 2019

Author Contributor

Same comment as above.

Malia Guerrero
@kibertoad kibertoad merged commit 03d6f06 into knex:master Nov 21, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 21, 2019

Thanks!

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 27, 2019

Released in 0.20.3

@noisyui

This comment has been minimized.

Copy link

noisyui commented Dec 4, 2019

A related bug is that calling .stream() on a query to Oracle db return each chunk as Array, and the buffer column is returned as a Lob object as an entry in the array. And the query context is not repected when calling the .stream() method, no mattter for Oracle or PostgreSQL.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Dec 4, 2019

@noisyui Would you consider sending a PR?

@noisyui

This comment has been minimized.

Copy link

noisyui commented Dec 5, 2019

@kibertoad I hope I can, but I don't think I'm the right person to do this at this moment, who grab this library for just several days. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.