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

JSONB and JSON type fields values should be untouched! #16

Closed Kostanos closed 5 years ago

Kostanos commented 5 years ago

Apparently this "plugin" also camelcasing the JSON inside the field of JSON type, which limits a lot of it uses.

As, I have some JSONs coming from third party modules, and I can "touch" them.

Kequc commented 5 years ago

The recursive nature of this library is because there are ways of returning deeply nested results from sql. Is there a way to detect that it is a json field returned from knex? Perhaps not have knex automatically parse json for example.

I'm not sure it's possible to make everyone happy.

Kequc commented 5 years ago

Which 3rd party modules are you using?

Kequc commented 5 years ago

Something I have been thinking about is replacing the ignoreStringcase option with an object where you can define a structure. For example:

{
    users: {
        field_to_ignore: true
    }
}

or maybe:

{
    users: ['field_to_ignore']
}

and it'd still work for people returning nested results like:

{
    users: {
        some_nested_result: ['field_to_ignore']
    }
}

Then the library would ignore that field and return a value from it as is. I wonder if that would be better than the dynamic way it is now.

Kostanos commented 5 years ago

Completely agree with you, it will be difficult or impossible to separate JSON field vs other type of object, perhaps, currently I found a workaround, and did something like this:

  ignoreStringcase: (obj) => !('id' in obj || 'product_id' in obj),

As in general, in my case, the field's JSON data use to not have 'id' or other '*_id' related fields, so this workaround works for me for now.

We can close this issue if you wish. Or keep discussing about ignoring field names as you mentioned.

Kequc commented 5 years ago

Or maybe... I could pass a second parameter to ignoreStringcase() which is the current path like... "users.field_to_ignore". Then if you really wanted you could use it to return true or false like.

ignoreStringcase: (obj, path) => path === 'users.field_to_ignore'

I remember trying this at one point and not being happy with the implementation. I'm not sure it would be easy. If I remember right the biggest problem is I couldn't figure out which table the result was coming from.

Otherwise, exactly what you did is what I was going to suggest I just wanted to keep my comments brief.

I suppose the third option would be for the lib to read from the knexjs schema somehow and try to interpret field values from that. But I'd have to look into it more deeply. Again I think not knowing the table is the biggest limitation.

I'll leave this open for a week or so in case anyone has suggestions.

Kequc commented 5 years ago

I've released 1.2.4 that adds some more parameters to ignoreStringcase.

The second parameter is the field name, so for example if all of your json fields end in "_json" maybe you could:

ignoreStringcase: (obj, name) => name.endsWith('_json')

The third parameter is the queryContext. As long as you define a queryContext in your request maybe you could use it to specify fields to ignore on the fly like so:

knex('users')
  .queryContext({ ignore: ['my_field'] })
  .select();
function ignoreStringcase(obj, name, queryContext) {
    const ignore = (queryContext || {}).ignore || [];
    return ignore.includes(name);
}

Or however you want to use it. I'm not sure I've never been clear on the usefulness of queryContext but there it is. Let me know if this helps.

Kequc commented 5 years ago

If you don't want to use any nested features and for it to behave like your fork you can use the above new feature for that.

{
    ignoreStringcase: (obj, name) => name !== ''
}

It will immediately return any object that has a key so that should cover everything I think.