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

snakeCaseMappers works differently for objects and FieldExpressions #1089

Open
Tapppi opened this issue Sep 24, 2018 · 5 comments · May be fixed by #2625
Open

snakeCaseMappers works differently for objects and FieldExpressions #1089

Tapppi opened this issue Sep 24, 2018 · 5 comments · May be fixed by #2625
Labels

Comments

@Tapppi
Copy link

Tapppi commented Sep 24, 2018

Using snakeCaseMappers, patching an object will not map inner keys, only the first level (columns), but using a FieldExpression will map the json field references.

class TestModel extends Model {
  ...
  static columnNameMappers = snakeCaseMappers();
  ...
}

// Query with array/object
TestModel
  .query()
  .patch({
    jsonColumn: [{
      innerKey: 1
    }]
  });

// Database output
{
  json_column: '[{"innerKey": 1}]' 
}

// Query with FieldExpression
TestModel
  .query()
  .patch({
    'jsonColumn:[0][innerKey]': 1
  });

// Database output
{
  json_column: '[{"inner_key": 1}]' 
}

EDIT: IMHO, this should work consistently across, and there could be an option for snakeCaseMappers | knexSnakeCaseMappers, something like deep|deepKeys|innerKeys. Obviously I would prefer it to default to false, and I'm guessing the usual expectation is around how the normal objects now work, but the default's not a big deal for me eitherway.

@koskimas
Copy link
Collaborator

koskimas commented Sep 25, 2018

Objection shouldn't touch the JSON fields at all. It's a bug that the inner keys actually do change in some case.

@koskimas koskimas added the bug label Sep 25, 2018
@Tapppi
Copy link
Author

Tapppi commented Sep 25, 2018

@koskimas agreed, the option would've been an additional feature based on the "hidden feature" ;)

I fixed this for us by:

  • copying the snakeCase from lib/utils/identifierMapping.js
  • creating a new snakeCaseMappers() factory that:
    • uses objection.snakeCaseMappers().parse() for parsing
    • uses the copied snakeCase for formatting, but only until the first :. Like mapLastPart, but until instead of from the separator

flipace pushed a commit to flipace/objection.js that referenced this issue May 7, 2019
koskimas added a commit that referenced this issue May 18, 2019
@koskimas
Copy link
Collaborator

I needed to revert the fix for this. It caused more problems than it fixed.

@koskimas koskimas reopened this May 18, 2019
@mindnektar
Copy link

any news on this? (thanks for your continued great work, by the way.)

@cesumilo
Copy link
Contributor

@lehni I did a proposal for this issue 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants