Raise error if list or map not provided #443

dobrynin · 2018-10-15T21:42:58Z

It seems to me that the existing conditional (value === undefined && value[0] === undefined) errors if value is indeed undefined. This change fixes that and adds a more descriptive error if the user forgets to provide the list key when using type: 'list'.

Summary:

Code sample:

Schema

// Code here

Model

// Code here

General

// Code here

GitHub linked issue:

#---

Other information:

Type (select 1):

Bug fix
Feature implementation
Documentation improvement
Testing improvement

Test added to report bug (GitHub issue #--- @---)
Something not listed here

Is this a breaking change? (select 1):

🚨 YES 🚨
No
I'm not sure

Is this ready to be merged into Dynamoose? (select 1):

Yes
No

Are all the tests currently passing on this PR? (select 1):

Yes
No

Other:

I have searched through the GitHub pull requests to ensure this PR has not already been submitted
I have updated the Dynamoose documentation (if required) given the changes I made
I have added/updated the Dynamoose test cases (if required) given the changes I made
I have run npm test from the root of the project directory to ensure all tests continue to pass
I agree that all changes made in this pull request may be distributed and are made available in accordance with the Dynamoose license
All of my commits and commit messages are detailed, explain what changes were made, and are easy to follow and understand
I have confirmed that all my code changes are indented properly using 2 spaces
I have filled out all fields above

fishcharlie · 2018-10-15T21:58:24Z

lib/Attribute.js

@@ -56,10 +56,11 @@ function Attribute(schema, name, value) {
  else if (this.type.name === 'list'){

    if(value.type) {
+      if (!value.list) throw new errors.SchemaError('No list schema provided for attribute:' + this.name );


Just for formatting sake, can we change this to the following?

if (!value.list){ throw new errors.SchemaError('No list schema provided for attribute:' + this.name); }

fishcharlie · 2018-10-15T21:59:27Z

lib/Attribute.js

      value = value.list;
    }

-    if (value === undefined && value[0] === undefined){
+    if (value === undefined || value[0] === undefined){


Since you said this was failing before, can we write a test that will fail in the current version, and pass with this PR to ensure this will work correctly in the future?

I'm now running into an issue where if I fetch a record from dynamo that has a field set to an empty array, this error gets raised. Is that the intended behavior?

@dobrynin Honestly I'm not sure. Are you running into that on the NPM version or only after this change? I believe this code should only be run when creating or setting up the schema/model. So I'm not sure why fetching a record from Dynamo would cause this code to be run unless you have saveUnknown set to true.

Can you try logging some things out such as value and see what that prints?

You can also run export DEBUG=* which will give you more console logs and details about what Dynamoose is doing behind the scenes.

Your thinking was right, we do set saveUnknown to true.

fishcharlie · 2018-11-12T01:33:26Z

@dobrynin You get a chance to work on this anymore? 😃

dobrynin · 2018-11-12T06:11:41Z

Sorry, I’ve been preoccupied with other work and have neglected this. I’ll get that test going ASAP

fishcharlie · 2018-11-12T18:22:04Z

@dobrynin Awesome! Let me know if you have any questions or if I can help at all.

It seems to me that the existing conditional `(value === undefined && value[0] === undefined)` errors if value is indeed undefined. This change fixes that and adds a more descriptive error if the user forgets to provide the `list` key when using `type: 'list'`.

coveralls · 2018-11-19T07:22:08Z

Pull Request Test Coverage Report for Build 710

4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.04%) to 85.145%

Files with Coverage Reduction	New Missed Lines	%
lib/Table.js	2	76.76%

Totals
Change from base Build 700:	-0.04%
Covered Lines:	1882
Relevant Lines:	2142

💛 - Coveralls

coveralls · 2018-11-19T07:22:09Z

Pull Request Test Coverage Report for Build 710

4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.04%) to 85.145%

Files with Coverage Reduction	New Missed Lines	%
lib/Table.js	2	76.76%

Totals
Change from base Build 700:	-0.04%
Covered Lines:	1882
Relevant Lines:	2142

💛 - Coveralls

coveralls · 2018-11-19T07:22:09Z

Pull Request Test Coverage Report for Build 805

4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
211 unchanged lines in 5 files lost coverage.
Overall coverage increased (+0.3%) to 85.474%

Files with Coverage Reduction	New Missed Lines	%
lib/index.js	17	87.03%
lib/Query.js	19	88.33%
lib/Scan.js	34	84.78%
lib/Table.js	41	78.59%
lib/Model.js	100	83.62%

Totals
Change from base Build 700:	0.3%
Covered Lines:	2150
Relevant Lines:	2440

💛 - Coveralls

fishcharlie · 2018-11-28T03:08:42Z

@dobrynin Are you ready for this to be re-reviewed for the potential of getting it merged in?

dobrynin · 2018-11-28T05:21:39Z

Yes, please do!

fishcharlie

Super close on this. Great work. Just a few more things!

fishcharlie · 2018-11-28T05:41:37Z

lib/Attribute.js

@@ -56,12 +59,12 @@ function Attribute(schema, name, value) {
  else if (this.type.name === 'list'){

    if(value.type) {
+      if (value.list === undefined || value.list[0] === undefined){


Above we use !value.map, here we use value.list === undefined and value.list[0] === undefined. Is there a reason this line isn't just (!value.list || !value.list[0])

fishcharlie · 2018-11-28T05:43:19Z

test/Schema.js

+      });
+    } catch (err) {
+      err.should.be.instanceof(Error);
+      err.should.be.instanceof(errors.SchemaError);


Not sure I'm a huge fan of this. If there is no error thrown, this test will still pass. I think it'd be better to create a variable above (set to nothing by default). Then inside the catch, set err to that variable we created above. Then right before done is called, run the assertions, to ensure that variable we created exists, ensure it's an instance of, etc.

fishcharlie · 2018-11-28T05:43:33Z

test/Schema.js

+      });
+    } catch (err) {
+      err.should.be.instanceof(Error);
+      err.should.be.instanceof(errors.SchemaError);


Same thing here that I mentioned above

fishcharlie · 2018-11-28T05:43:36Z

test/Schema.js

+      });
+    } catch (err) {
+      err.should.be.instanceof(Error);
+      err.should.be.instanceof(errors.SchemaError);


Same thing here that I mentioned above

fishcharlie · 2018-11-28T05:45:05Z

lib/Attribute.js

@@ -56,12 +59,12 @@ function Attribute(schema, name, value) {
  else if (this.type.name === 'list'){

    if(value.type) {
+      if (value.list === undefined || value.list[0] === undefined){
+        throw new errors.SchemaError('No list schema given for attribute:' + this.name );


Suggested change

throw new errors.SchemaError('No list schema given for attribute:' + this.name );

throw new errors.SchemaError(`No list schema given for attribute: ${this.name}`);

Super picky, I know. But PR #465 is doing this, so I think from now on we should do it throughout the code.

fishcharlie · 2018-11-28T05:45:44Z

lib/Attribute.js

@@ -42,6 +42,9 @@ function Attribute(schema, name, value) {
  if (this.type.name === 'map'){

    if(value.type) {
+      if (!value.map) {
+        throw new errors.SchemaError('No map schema given for attribute:' + this.name );


Suggested change

throw new errors.SchemaError('No map schema given for attribute:' + this.name );

throw new errors.SchemaError(`No map schema given for attribute: ${this.name}`);

Super picky, I know. But PR #465 is doing this, so I think from now on we should do it throughout the code.

fishcharlie · 2018-12-11T23:23:42Z

@dobrynin Any progress on this? I'd love to get it into version 1.3.0 (#481) if possible, but also not going to hold up the release for this PR.

dobrynin · 2018-12-16T04:00:30Z

@fishcharlie, sorry for the late reply, I've implemented your changes.

New changes

fishcharlie

LGTM

fishcharlie requested changes Oct 15, 2018

View reviewed changes

fishcharlie added the status:do-not-merge label Oct 15, 2018

dobrynin and others added 2 commits November 18, 2018 21:00

Add missing map error and tests

7a428de

Add empty array test

6a209d1

fishcharlie previously requested changes Nov 28, 2018

View reviewed changes

dobrynin added 2 commits December 15, 2018 19:59

Tweak missing list conditional and use template literals

f7e600c

Improve missing list/map tests

59782d0

fishcharlie removed the status:do-not-merge label Dec 16, 2018

fishcharlie approved these changes Dec 16, 2018

View reviewed changes

fishcharlie changed the title ~~Raise error if list not provided~~ Raise error if list or map not provided Dec 16, 2018

fishcharlie mentioned this pull request Dec 16, 2018

Version 1.3.0 #481

Merged

fishcharlie merged commit 91bacda into dynamoose:master Dec 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Raise error if list or map not provided #443

Raise error if list or map not provided #443

dobrynin commented Oct 15, 2018 •

edited

Loading

fishcharlie Oct 15, 2018

fishcharlie Oct 15, 2018

dobrynin Oct 16, 2018

dobrynin Oct 18, 2018

fishcharlie Oct 18, 2018

dobrynin Oct 18, 2018 •

edited

Loading

fishcharlie commented Nov 12, 2018

dobrynin commented Nov 12, 2018

fishcharlie commented Nov 12, 2018

coveralls commented Nov 19, 2018

coveralls commented Nov 19, 2018

coveralls commented Nov 19, 2018 •

edited

Loading

fishcharlie commented Nov 28, 2018

dobrynin commented Nov 28, 2018

fishcharlie left a comment

fishcharlie Nov 28, 2018

fishcharlie Nov 28, 2018

fishcharlie Nov 28, 2018

fishcharlie Nov 28, 2018

fishcharlie Nov 28, 2018

fishcharlie Nov 28, 2018

fishcharlie commented Dec 11, 2018 •

edited

Loading

dobrynin commented Dec 16, 2018

fishcharlie left a comment

	throw new errors.SchemaError('No list schema given for attribute:' + this.name );
	throw new errors.SchemaError(`No list schema given for attribute: ${this.name}`);

	throw new errors.SchemaError('No map schema given for attribute:' + this.name );
	throw new errors.SchemaError(`No map schema given for attribute: ${this.name}`);

Raise error if list or map not provided #443

Raise error if list or map not provided #443

Conversation

dobrynin commented Oct 15, 2018 • edited Loading

Summary:

Code sample:

Schema

Model

General

GitHub linked issue:

Other information:

Type (select 1):

Is this a breaking change? (select 1):

Is this ready to be merged into Dynamoose? (select 1):

Are all the tests currently passing on this PR? (select 1):

Other:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dobrynin Oct 18, 2018 • edited Loading

Choose a reason for hiding this comment

fishcharlie commented Nov 12, 2018

dobrynin commented Nov 12, 2018

fishcharlie commented Nov 12, 2018

coveralls commented Nov 19, 2018

Pull Request Test Coverage Report for Build 710

💛 - Coveralls

coveralls commented Nov 19, 2018

Pull Request Test Coverage Report for Build 710

💛 - Coveralls

coveralls commented Nov 19, 2018 • edited Loading

Pull Request Test Coverage Report for Build 805

💛 - Coveralls

fishcharlie commented Nov 28, 2018

dobrynin commented Nov 28, 2018

fishcharlie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fishcharlie commented Dec 11, 2018 • edited Loading

dobrynin commented Dec 16, 2018

fishcharlie left a comment

Choose a reason for hiding this comment

dobrynin commented Oct 15, 2018 •

edited

Loading

dobrynin Oct 18, 2018 •

edited

Loading

coveralls commented Nov 19, 2018 •

edited

Loading

fishcharlie commented Dec 11, 2018 •

edited

Loading