Kequc / knex-stringcase

Helper switches key case for npm knex
https://www.npmjs.com/package/knex-stringcase
ISC License
52 stars 8 forks source link

Not working properly for sqlite3 in-memory DB #21

Closed xmany closed 4 years ago

xmany commented 4 years ago

Hi, I've encountered a problem when using knexStringcase for sqlite3 in-memory db, the property Key is not parsed from snake case to camel case.

  db = Knex(
    knexStringcase({
      client: "sqlite3",
      connection: {
        filename: ":memory:",
      },
    })
  );

const result = db(table)
      .select("*")
      .where(where)
      ...

console.log(result[0]);

I got:

{
    id: '00000000-0000-0000-0000-000000000003',
    tenant_id: '94463c40-8e56-4afd-bee5-d2eda624bee9',
    created_at: 1584368640000
}

It works correctly when using a real postgres DB

Kequc commented 4 years ago

Thanks.

Is that really the only difference, just using sqlite3? I've not run into that before. As long as knex is running the wrapIdentifier and postProcessResponse functions, and as long as what it's returning from the database are objects (or arrays), without some weird nesting involved. I'd expect to see it work.

What if you added recursiveStringcase: () => true to your config?

  db = Knex(
    knexStringcase({
      client: "sqlite3",
      connection: {
        filename: ":memory:",
      },
      recursiveStringcase: () => true
    })
  );
xmany commented 4 years ago

Hi @Kequc , thanks for your reply!

However your suggestion is not working for me, After I do this:

  db = Knex(
    knexStringcase({
      client: "sqlite3",
      connection: {
        filename: ":memory:",
      },
      recursiveStringcase: () => true,
    })
  );

I still got: (sadly)

{ 
  id: '00000000-0000-0000-0000-000000000003',
  tenant_id: '55dcd526-4c4a-4a44-821e-70f69437922c',
  created_at: 1584368640000 
}
Kequc commented 4 years ago

I took a look and there aren't any open issues on their tracker regarding these methods not being run. Not to force you to debug instead of me but does this do anything in your code?

  db = Knex(
    client: "sqlite3",
    connection: {
      filename: ":memory:",
    },
    postProcessResponse(result) {
      console.log(result)
      return { actually: "It is running these functions." }
    }
  );

That doesn't use knexStringcase at all and should always return (to any db query):

{
  actually: "It is running these functions."
}

Please let me know what that console.log is outputting as well.

xmany commented 4 years ago

Hi @Kequc , When doing this:

  db = Knex(
    client: "sqlite3",
    connection: {
      filename: ":memory:",
    },
    postProcessResponse(result) {
      console.log(result)
      return { actually: "It is running these functions." }
    }
  );

I got this from the console log:

[ { id: '00000000-0000-0000-0000-000000000000',
     tenant_id: '00000000-0000-0000-0000-000000000001',
     created_at: 1598941518 } ]

My own converter works, I made my own converter like this:

    postProcessResponse: result => {
      if (Array.isArray(result)) {
        return result.map(row => snakeToCamel(row));
      }
      return snakeToCamel(result);
    },
Kequc commented 4 years ago

Something doesn't make sense. You say this thing works for postgresql but not sqlite, but if they both output the same data then both should work.

Because my lib is just adding the postProcessResponse and wrapIdentifier methods which take arrays/objects as a parameter. There should be no difference.

I thought maybe your database was returning oddly formatted data, or maybe that knex wasn't running these methods. But it all looks normal to me.

Kequc commented 4 years ago

I'm closing this for now.

If anyone is having this issue feel free to re-open it! But I think I'm unable to replicate the problem for now.

jkamzi commented 4 years ago

I have run in to the same issue as described above. The issue happens when testing using jest. (Note: The issue is most likely isolated to jest: https://github.com/facebook/jest/issues/2549). I did make a test for mocha, no issue there.

The following line is where jest breaks:

https://github.com/Kequc/knex-stringcase/blob/1637dfbb6b6ca56f65d7ee30c5bd56c034f0bd42/src/key-converter-factory.js#L24

However, I wonder if this line could not be changed to:

// https://stackoverflow.com/a/8511332
if(typeof value !== 'object' && value !== 'null')

Demo repo: https://github.com/jkamzi/knex-stringcase-issues-21

Env:

  Binaries:
    Node: 14.10.1 - /usr/local/bin/node
    npm: 6.14.8 - /usr/local/bin/npm
  npmPackages:
    jest: ^26.4.2 => 26.4.2 
Kequc commented 4 years ago

Is that the problem.

In some ways I feel like that's a big problem in Jest but at the same time that bug has been open since 2017. I mean is the entire Javascript ecosystem meant to stop using instanceof because of Jest? And how is jest creating objects that aren't instances of objects? That doesn't seem possible.

I'm open to switching to something else. How would we handle the line under it?

if (value instanceof Date) return true;
Kequc commented 4 years ago

I've released 1.4.3 which specifically addresses more robust object detection. @xmany I really hope this fixes your issue as well. Thank you guys for the stackoverflow link. I suppose today I learned instanceof Object doesn't work on objects created using Object.create(null), nobody ever told me that.

Maybe that's the problem plaguing Jest. In that case it might be particularly about the way jest is making use of Object.create and your database returning null's. Which would then skip processing the keys returned from the database on that object. But since it was null anyway I don't know why it matters.

Probably shouldn't be affecting actual objects returned from the database either way

In any case, hopeful it's a fix.

jkamzi commented 4 years ago

@Kequc I updated the demo repo to use postgres to test with dates. I noticed right away that the test were passing when using postgres but failed with sqlite3. This was with v 1.4.2. It seems that there's a weird edge case when using sqlite3 and jest. I did test using mocha and it worked without issues

Kequc commented 4 years ago

So therefore there is still an issue detecting dates?

If that's the case I suppose we could do a type of date check using typeof value.getMonth === 'function' that couldn't fail I don't think. As it wouldn't be possible for knex to return a function from the database.

Playing round in the console.

var obj = Object.create(null)
obj instanceof Object #=> false
obj.someValue = true
obj #=> {someValue: true}
obj instanceof Object #=> false

var dat = Object.create(new Date())
dat instanceof Object #=> true
typeof dat.getMonth #=> 'function'
dat #=> Date {}
dat instanceof Date #=> true

Working with dates seems okay.

jkamzi commented 4 years ago

As the issue seem to be isolated to sqlite (tested with sqlite, postgres and mysql) i think it should be fine as it is now. I don't believe sqlite3 can return native JS dates.

edit: Did a bit more digging. This issue is isolated to jest and sqlite3. It is jest use of vm that is causing the issue.

Kequc commented 4 years ago

Did the new version (1.4.3) fix it at least?

jkamzi commented 4 years ago

Yep 1.4.3 fixed the issue

Kequc commented 4 years ago

Awesome

jkamzi commented 4 years ago

Ty for fixing it @Kequc

xmany commented 4 years ago

Thanks @Kequc for fixing it, I'll update the version and test it out.

Yes it is happening with jest, I was not aware jest has a different javascript environment, e.g. instanceof issue. Thanks @jkamzi for pointing out and help solve the problem!