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

Allow $select to catch and use string syntax #158

Merged
merged 5 commits into from
Jan 13, 2017

Conversation

jamesjnadeau
Copy link
Contributor

@jamesjnadeau jamesjnadeau commented Jan 13, 2017

mongoose allows a string to be passed to the .select() function ie. select('+someExtraField').

Using this syntax allows you to keep all existing fields selected, but select additional fields you might have hidden in your schema by using select: false as a "column/field" option. It's also a more succinct and flexible way of passing the select information IMHO.

I hope that this change will allow the other forms to be used, but allow this use case.

I still need to check/write tests and clean up the code a bit, but getting this out early for concerns/comment.

mongoose allows a string to be passed to the `.select()` function ie. `select('+someExtraField')`. 

Using this syntax allows you to keep all existing fields selected, but select additional fields you might have hidden in your schema by using `select: false` as a "column/field" option. 

I hope that this change will allow the other forms to be used, but still catch this case. I still need to check/write tests
@jamesjnadeau jamesjnadeau changed the title 🐛 fix $select parsing to allow string syntax Allow $select parsing to allow string syntax Jan 13, 2017
@jamesjnadeau jamesjnadeau changed the title Allow $select parsing to allow string syntax Allow $select to catch and use string syntax Jan 13, 2017
@@ -37,7 +37,9 @@ class Service {
const q = this.Model.find(query).lean(this.lean);

// $select uses a specific find syntax, so it has to come first.
if (filters.$select && filters.$select.length) {
if (typeof filters.$select === 'string') {
Copy link
Member

Choose a reason for hiding this comment

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

I think you might be able to just add this check to line 51, which would pass the string to the mongoose query.select()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think I can because of how the current check works on line 43, a string has a length right?(I just double checked in my console :)

I could modify it, but I'd rather leave it alone....

@jamesjnadeau
Copy link
Contributor Author

jamesjnadeau commented Jan 13, 2017

ok, I've added test for $select on strings, arrays and objects. Works for me! thanks so much for you time and this wonderful codebase!

@marshallswain
Copy link
Member

marshallswain commented Jan 13, 2017

Can you try this with your tests?

if (filters.$select) {
  // $select uses a specific find syntax, so it has to come first.
  if (typeof filters.$select === 'string' || typeof filters.$select === 'object') {
    q.select(filters.$select);
  } else if (Array.isArray(filters.$select)) {
    let fields = {};

    for (let key of filters.$select) {
      fields[key] = 1;
    }

    q.select(fields);
  }
}

I think it's actually more lines of code, but it's a bit cleaner.

@marshallswain
Copy link
Member

Actually, I don't think the outer if is even needed:

// $select uses a specific find syntax, so it has to come first.
if (typeof filters.$select === 'string' || typeof filters.$select === 'object') {
  q.select(filters.$select);
} else if (Array.isArray(filters.$select)) {
  let fields = {};

  for (let key of filters.$select) {
    fields[key] = 1;
  }

  q.select(fields);
}

@jamesjnadeau
Copy link
Contributor Author

@marshallswain I've updated the logic to simplify it, take a look when you have a chance, thanks!

Close to what you had suggested, but I found the array needs to be checked first because an array is an object.

@marshallswain
Copy link
Member

marshallswain commented Jan 13, 2017

Oh right. Looks good to me.

@daffl daffl merged commit 756d7bc into feathersjs-ecosystem:master Jan 13, 2017
@daffl
Copy link
Member

daffl commented Jan 13, 2017

Nice, I didn't know this was possible. Released as v3.6.2. Thanks @jamesjnadeau!

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.

3 participants