Ability to define normalization of undefined query #725

jrschumacher · 2015-09-30T00:53:01Z

Add setting to datasource normalizeUndefinedInQuery to determine how
it will handle undefined values. Options:

setNull : converts undefined to null
throwError : throw an error on undefined value
strip : strip the key where undefined value is found

Defualt operation is to strip the key.

Add setting to datasource `normalizeUndefinedInQuery` to determine how it will handle undefined values. Options: - setNull : converts undefined to null - throwError : throw an error on undefined value - strip : strip the key where undefined value is found Defualt operation is to strip the key.

slnode · 2015-09-30T00:53:02Z

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

bajtos · 2015-10-01T08:55:28Z

@slnode ok to test

bajtos · 2015-10-01T09:03:39Z

Hi @jrschumacher, thank you for the patch!

I would personally prefer different option names.

IMO we should rename throwError to throw to make it consistent with the existing model option strict which allows the value throw - see https://github.com/strongloop/loopback-datasource-juggler/blob/c4de830bfe4e4a4aa704b467c4d6c037665bb091/lib/model.js#L227-L228.
I'd prefer nullify over setNull, but I admit this is subjective.
As for strip, I'd rather use remove or delete, but again, this is subjective too.

Few more ideas to consider:

Should we make this option configurable at Model level too?
Should we take into account Model option persistUndefinedAsNull (Add model setting "persistUndefinedAsNull" #542) and set handleUndefined='setNull' when normalizeUndefinedInQuery is not explicitly set?

I'm fine to leave those two out of scope of this PR though.

bajtos · 2015-10-01T09:04:42Z

@raymondfeng since you are more familiar with the issue being fixed, I'd like to leave review & approval for you to make.

jrschumacher · 2015-10-01T15:03:50Z

@bajtos I'm happy to change the naming. Will fix later today also I'll look into exploring the model level settings. Questions regarding that: if you set data source level should model level overwrite? Is there an order of precedence?

jrschumacher · 2015-10-01T15:06:28Z

@bajtos regarding the persistUndefinedToNull would it be better to change the datasource setting name to be similar in naming? Or does persist refer more to the longevity of the data in the datasource rather than the transient query?

raymondfeng · 2015-10-01T15:28:04Z

Here is my take:

Make the configuration available at multiple levels (preferably app & datasource level)
1. app (server/config.json, global to all models within the app)
2. datasource (connector settings, global to all models within the same ds. I'm not sure if it's necessary)
3. model (model settings, local to the model)
4. options for model methods (local to the request. I'm not sure if it's necessary)
Name/value of the config property:
- normalizeUndefinedInQuery
  - throw
  - nullify
  - skip (or ignore)

jrschumacher · 2015-10-01T15:29:38Z

@raymondfeng got it

jrschumacher · 2015-10-01T15:45:24Z

@raymondfeng I think the connector level will be valuable to MongoDB users. So maybe if you have a RDBM or redis datasource you will want the stripping effect but with MongoDB the stripping created some serious issues.

bajtos · 2015-10-01T16:05:54Z

👍 for @raymondfeng proposal.

if you set data source level should model level overwrite? Is there an order of precedence?

Yes, more-specific level should override less-specific level. I.e. model level overrides datasource level.

regarding the persistUndefinedToNull would it be better to change the datasource setting name to be similar in naming? Or does persist refer more to the longevity of the data in the datasource rather than the transient query?

You name is fine. persistUndefinedAsNull is a different thing - it controls how well-known fields with no value are persisted.

jrschumacher · 2015-10-05T03:05:13Z

@raymondfeng @bajtos could I get some help on how to fetch the app settings? I've created a method to get settings based on priority but need to add app settings: https://github.com/strongloop/loopback-datasource-juggler/pull/725/files#diff-d717a01e40d8164976e4468af1bce663R942

raymondfeng · 2015-10-05T03:52:08Z

this.app.get('propName') should work

jrschumacher · 2015-10-05T11:33:37Z

@raymondfeng I've done this, but I don't know how to add the tests for this. There doesn't seem to be a global app object.

bajtos · 2015-10-05T20:42:49Z

I'd rather not use app in the juggler, it's a concept from loopback. Since loopback-datasource-juggler is a dependee of loopback, it should not access loopback-specific properties, as that would break encapsulation and isolation of this component. Instead, loopback should inject normalizeUndefinedInQuery when attaching a model to an app - see https://github.com/strongloop/loopback/blob/e68cd8fc87730404a78e1475b828886e3582bd6f/lib/application.js#L156-L159 and https://github.com/strongloop/loopback/blob/e68cd8fc87730404a78e1475b828886e3582bd6f/lib/application.js#L156-L159

bajtos · 2015-10-05T20:43:55Z

Another approach is to override datasource settings when creating a new datasource in an app - see https://github.com/strongloop/loopback/blob/e68cd8fc87730404a78e1475b828886e3582bd6f/lib/application.js#L221-L227

jrschumacher · 2015-10-05T20:48:45Z

@bajtos then is there anything else that's needed from me here? I'm not sure why the above tests fail. Locally all the tests pass.

bajtos · 2015-10-05T20:55:22Z

Hmm, the tests are failing with this error:

  1) util.removeUndefined Remove undefined values from the query object:

      AssertionError: expected { where: { x: 1 } } deepEqual { where: { x: 1, y: null } }
      + expected - actual

       {
         "where": {
           "x": 1
      +    "y": [null]
         }
       }

      at Context.<anonymous> (test/util.test.js:71:12)

Could you please rebase your feature branch on top of the current master and run the tests locally again?

bajtos · 2015-10-05T20:55:52Z

lib/dao.js

+    return ds.settings[key];
+  }
+
+  // @TODO: Check for settings in app


Please remove per the conversation above.

@bajtos am I just removing the TODO or all of it?

Just the TODO. It would be nice if you could send a patch to LoopBack which will apply app settings to newly created datasources, but that's not necessary to get this patch landed.

jrschumacher · 2015-10-05T20:57:41Z

@bajtos yep I'll rebase.

Regarding the settings. Shall I still get model and datasource but do it in the normalize method?

bajtos · 2015-10-05T21:00:53Z

Shall I still get model and datasource but do it in the normalize method?

Your current approach LGTM, I left few mostly stylistic comments to address.

Added method to get settings based on priority

jrschumacher · 2015-10-05T21:31:12Z

@bajtos okay I think I handled all the issues you mentioned.

jrschumacher · 2015-10-05T21:32:57Z

It would be nice if you could send a patch to LoopBack which will apply app settings to newly created datasources, but that's not necessary to get this patch landed.

I'll consider this, but its not going to be anytime soon. Also I took note of the references you mentioned, but I have to say I'm a bit at a loss exactly what you're asking me to do. Is this specific to this setting or is it for all settings?

bajtos · 2015-10-07T16:29:27Z

Landed via 98fb3c6, thank you for the contribution!

Also I took note of the references you mentioned, but I have to say I'm a bit at a loss exactly what you're asking me to do. Is this specific to this setting or is it for all settings?

I'd modify app.datasource() to apply this specific setting only.

bajtos · 2015-10-07T20:01:23Z

Released as [email protected]

jrschumacher · 2015-10-08T03:22:00Z

Glad to help!

On Oct 7, 2015, 16:01 -0400, Miroslav Bajtoš[email protected], wrote:

Released [email protected]

—
Reply to this email directly orview it on GitHub(#725 (comment)).

altsang added the #community contribution label Sep 30, 2015

jrschumacher mentioned this pull request Sep 30, 2015

findOne() returns non-empty result if a value of filtering criteria is undefined strongloop/loopback#637

Closed

bajtos assigned raymondfeng Oct 1, 2015

bajtos reviewed Oct 5, 2015
View reviewed changes

Refactor naming and support model settings

5c40e48

Added method to get settings based on priority

jrschumacher force-pushed the feature-normalize-undefined-in-query branch from ef43a73 to 5c40e48 Compare October 5, 2015 21:29

bajtos closed this in 98fb3c6 Oct 7, 2015

bajtos removed the #community contribution label Oct 7, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ability to define normalization of undefined query #725

Ability to define normalization of undefined query #725

jrschumacher commented Sep 30, 2015

slnode commented Sep 30, 2015

bajtos commented Oct 1, 2015

bajtos commented Oct 1, 2015

bajtos commented Oct 1, 2015

jrschumacher commented Oct 1, 2015

jrschumacher commented Oct 1, 2015

raymondfeng commented Oct 1, 2015

jrschumacher commented Oct 1, 2015

jrschumacher commented Oct 1, 2015

bajtos commented Oct 1, 2015

jrschumacher commented Oct 5, 2015

raymondfeng commented Oct 5, 2015

jrschumacher commented Oct 5, 2015

bajtos commented Oct 5, 2015

bajtos commented Oct 5, 2015

jrschumacher commented Oct 5, 2015

bajtos commented Oct 5, 2015

bajtos Oct 5, 2015

jrschumacher Oct 5, 2015

bajtos Oct 5, 2015

jrschumacher commented Oct 5, 2015

bajtos commented Oct 5, 2015

jrschumacher commented Oct 5, 2015

jrschumacher commented Oct 5, 2015

bajtos commented Oct 7, 2015

bajtos commented Oct 7, 2015

jrschumacher commented Oct 8, 2015

Ability to define normalization of undefined query #725

Ability to define normalization of undefined query #725

Conversation

jrschumacher commented Sep 30, 2015

slnode commented Sep 30, 2015

bajtos commented Oct 1, 2015

bajtos commented Oct 1, 2015

bajtos commented Oct 1, 2015

jrschumacher commented Oct 1, 2015

jrschumacher commented Oct 1, 2015

raymondfeng commented Oct 1, 2015

jrschumacher commented Oct 1, 2015

jrschumacher commented Oct 1, 2015

bajtos commented Oct 1, 2015

jrschumacher commented Oct 5, 2015

raymondfeng commented Oct 5, 2015

jrschumacher commented Oct 5, 2015

bajtos commented Oct 5, 2015

bajtos commented Oct 5, 2015

jrschumacher commented Oct 5, 2015

bajtos commented Oct 5, 2015

bajtos Oct 5, 2015

Choose a reason for hiding this comment

jrschumacher Oct 5, 2015

Choose a reason for hiding this comment

bajtos Oct 5, 2015

Choose a reason for hiding this comment

jrschumacher commented Oct 5, 2015

bajtos commented Oct 5, 2015

jrschumacher commented Oct 5, 2015

jrschumacher commented Oct 5, 2015

bajtos commented Oct 7, 2015

bajtos commented Oct 7, 2015

jrschumacher commented Oct 8, 2015