fix: return undefined for first level undefined props #138

b-admike · 2019-02-19T21:41:08Z

Description

Keep backward-compatilibity from #136 so that for first level properties, we return what we did before.

Fixes loopbackio/loopback-connector-postgresql#364

Checklist

New tests added or existing tests modified to cover all changes
Code conforms with the style
guide

jannyHou

@b-admike Nice. Your change LGTM and test is good.
PS: Please fix the commit message error before merging.
Verified the cloudant/couchdb2/mongodb failures are not related.

b-admike · 2019-02-20T04:08:27Z

@pierissimo What is your take on this change? I'm not entirely convinced that this is the right direction.

pierissimo · 2019-02-20T08:00:35Z

I'm not familiar with the postgres connector, and why is trying to access non-defined property definition.
The solution proposed looks good, I wonder if we should return undefined even for nested properties, for consistency.

PS: Honestly I feel that throwing an exception in this case is correct, but I understand that it's not backward compatible.

b-admike · 2019-02-20T16:09:16Z

I'm not familiar with the postgres connector, and why is trying to access non-defined property definition.

For trying to access undefined property definitions, there is a bug in the connector which I think I have a fix at loopbackio/loopback-connector-postgresql#365. But, we also returned the property definition as is before (even if it was undefined), so in some cases it might be working as expected.

The solution proposed looks good, I wonder if we should return undefined even for nested properties, for consistency.

Yes that would be better.

PS: Honestly I feel that throwing an exception in this case is correct, but I understand that it's not backward compatible.

I agree with this. I have pulled the released version 4.6.0 back so we can assess how to proceed with #136 for release. I think we should release it as a semver major since it has the breaking changes (I feel we can improve on the error message depending on the condition, but that can be done in a follow-up PR). If we decide to go ahead with this patch, then we can release a new version which is a semver minor and still backward-compatible. @strongloop/ch-lb-db-connectors @strongloop/sq-lb-apex Thoughts?

raymondfeng · 2019-02-20T16:15:55Z

test/connector.test.js

+    it('should preserve backward-compatibility for non-existing property', () => {
+      const definition = connector.getPropertyDefinition('MyModel', 'idontexist');
+      // eslint-disable-next-line no-unused-expressions
+      expect(definition).to.be.undefined;


Does it allow expect(definition).to.be.undefined();?

I tried, but it doesn't according to the docs. Another option is to do expect(definition).to.be.an('undefined');

@b-admike Have you considered the following?

expect(definition).to.equal(undefined);

Beware of libraries that assert on property access!

In the past, I have had good experience with using dirty-chai plugin for chai, see how we are using it in LoopBack 3.x here: loopback's test/helpers/expect.js

raymondfeng · 2019-02-20T16:17:38Z

lib/connector.js

@@ -155,12 +155,12 @@ Connector.getNestedPropertyDefinition = function(definition, propPath) {
  const isArray = !isPropUndefined && Array.isArray(prop.type);
  const isFunction = !isPropUndefined && !isArray && typeof prop.type === 'function';

+  if (propPath.length === 1) return prop;
+
  if (isPropUndefined || (propPath.length > 1 && (isArray && prop.type.length === 0))) {
    throw new Error(g.f('Invalid property path'));
  }


This method is to retrieve metadata. IMO, we should return undefined if no property definition exists for such path and have caller of this function to throw if necessary.

pierissimo · 2019-02-20T16:21:09Z

I think we should release it as a semver major since it has the breaking changes (I feel we can improve on the error message depending on the condition, but that can be done in a follow-up PR). If we decide to go ahead with this patch, then we can release a new version which is a semver minor and still backward-compatible. @strongloop/ch-lb-db-connectors @strongloop/sq-lb-apex Thoughts?

@b-admike In that case, we could just add a return statement here (line 161), instead of throwing the error:
https://github.com/strongloop/loopback-connector/pull/138/files#diff-d756fa1c3234311d61ddb0dc4e6630b0R161

b-admike · 2019-02-20T17:16:16Z

Okay, I think we are all good with landing this patch and releasing a minor/patch to npm. Thank you all for the feedback/reviews.

Return early for non-nested properties as before, and undefined instead of erroring out when the property type is undefined.

b-admike self-assigned this Feb 19, 2019

b-admike requested review from dhmlau, jannyHou and virkt25 as code owners February 19, 2019 21:41

jannyHou reviewed Feb 19, 2019

View reviewed changes

b-admike mentioned this pull request Feb 20, 2019

fix: use original prop name to search fields loopbackio/loopback-connector-postgresql#365

Closed

2 tasks

b-admike mentioned this pull request Feb 20, 2019

Connector fails during initialization - Invalid property path for getNestedPropertyDefinition loopbackio/loopback-connector-postgresql#364

Closed

raymondfeng reviewed Feb 20, 2019

View reviewed changes

b-admike force-pushed the fix/reserve-bc-propdef branch from b3eb823 to a9d4a3f Compare February 20, 2019 17:09

raymondfeng approved these changes Feb 20, 2019

View reviewed changes

pierissimo referenced this pull request Feb 21, 2019

feat: retrieve nested property definitions

30c32b5

b-admike force-pushed the fix/reserve-bc-propdef branch from a9d4a3f to 340f397 Compare February 23, 2019 03:33

b-admike requested a review from emonddr as a code owner February 23, 2019 03:33

fix: make prop def getter backward-compatible

95b907b

Return early for non-nested properties as before, and undefined instead of erroring out when the property type is undefined.

b-admike force-pushed the fix/reserve-bc-propdef branch from 340f397 to 95b907b Compare February 23, 2019 03:46

jannyHou approved these changes Feb 25, 2019

View reviewed changes

b-admike deleted the fix/reserve-bc-propdef branch February 25, 2019 19:57

b-admike merged commit 386e041 into master Feb 25, 2019

b-admike mentioned this pull request Feb 25, 2019

test: remove decimal coercion from queries loopbackio/loopback-connector-mongodb#498

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: return undefined for first level undefined props #138

fix: return undefined for first level undefined props #138

b-admike commented Feb 19, 2019

jannyHou left a comment •

edited

Loading

b-admike commented Feb 20, 2019

pierissimo commented Feb 20, 2019

b-admike commented Feb 20, 2019

raymondfeng Feb 20, 2019

b-admike Feb 20, 2019 •

edited

Loading

bajtos Feb 28, 2019

raymondfeng Feb 20, 2019

pierissimo commented Feb 20, 2019

b-admike commented Feb 20, 2019

fix: return undefined for first level undefined props #138

fix: return undefined for first level undefined props #138

Conversation

b-admike commented Feb 19, 2019

Description

Checklist

jannyHou left a comment • edited Loading

Choose a reason for hiding this comment

b-admike commented Feb 20, 2019

pierissimo commented Feb 20, 2019

b-admike commented Feb 20, 2019

raymondfeng Feb 20, 2019

Choose a reason for hiding this comment

b-admike Feb 20, 2019 • edited Loading

Choose a reason for hiding this comment

bajtos Feb 28, 2019

Choose a reason for hiding this comment

raymondfeng Feb 20, 2019

Choose a reason for hiding this comment

pierissimo commented Feb 20, 2019

b-admike commented Feb 20, 2019

jannyHou left a comment •

edited

Loading

b-admike Feb 20, 2019 •

edited

Loading