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.

Sign up for GitHub

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

Jump to bottom

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
bug

Comments

@Tapppi
Copy link

Tapppi commented Sep 24, 2018 •
edited

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 •
edited

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 •
edited

@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 @flipace
fixes Vincit#1089
1b4ecd8
This was referenced May 8, 2019
Closed
Closed
koskimas added a commit that referenced this issue May 18, 2019
@koskimas
revert fix for #1089
e27e6ec
@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.)

@wwei-flux wwei-flux mentioned this issue Nov 25, 2020
Closed
cesumilo added a commit to inarix/objection.js that referenced this issue Mar 15, 2024
@cesumilo
Fixes columnNameMappers applied to json column when using FieldExpres…
e5dc99c 
…sions (Vincit#1089)
cesumilo added a commit to inarix/objection.js that referenced this issue Mar 15, 2024
@cesumilo
Fixes columnNameMappers applied to json column when using FieldExpres…
a8c937f 
…sions (Vincit#1089)
@cesumilo cesumilo linked a pull request Mar 15, 2024 that will close this issue
Open
@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
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fixes columnNameMappers using FieldExpressions (#1089) inarix/objection.js
4 participants
@mindnektar @koskimas @Tapppi @cesumilo