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

Add flag to exclude instance properties from enumeration #724

Merged
merged 1 commit into from May 9, 2016

Conversation

flacnut
Copy link
Contributor

@flacnut flacnut commented May 6, 2016

We would like to be able to have certain properties not get serialized by JSON.stringify. Eg:

orm.define('user', {
    login: {
      type: 'text',
      required: true,
    },
    password: {
      type: 'text',
      required: true,
      enumerable: false   // <--- Supported with this PR
    },
   // ... other properties
});

expressApp.get('/user', (req, res) => {
  orm.user.get(req.userId, (err, user) => {
    if (err) { /*500*/ }
    res.send(user); // will call JSON.stringify - we don't want to send the password!
  });
});

Currently we have a serialize() method declared on the user definition which only includes properties we want. We call res.send(user.serialize()) but this isn't recursive, is quite unclean, and can potentially allow us to 'leak' secret properties if we are forgetful to call .serialize().

When this merges I will update the Wiki accordingly.

prop && prop.hasOwnProperty &&
prop.hasOwnProperty('enumerable') &&
!prop.enumerable
);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like this idea, but it would be better to handle this at the source as we do with required so that every property has enumerable set.

Also, this PR needs some tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, I'll move this code this week. I'll also add some tests (I tried to run them on my macbook but had some issues. I'll investigate further...)

@@ -519,7 +519,7 @@ function Instance(Model, opts) {
opts.changes.push(key);
}
},
enumerable: true
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prop may be undefined when key is value on a 'hasMany' relationship.

@flacnut
Copy link
Contributor Author

flacnut commented May 9, 2016

@dxg Updated - I was trying to find where the required: true tests are to put the enumerable: true tests beside it. This was the next best location I could find.

@dxg
Copy link
Collaborator

dxg commented May 9, 2016

👍

@dxg dxg merged commit e4df54b into dresende:master May 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants