Skip to content
This repository has been archived by the owner on Aug 29, 2018. It is now read-only.

Add support for dot notation, fix some whitespace #8

Merged
merged 4 commits into from
Jan 27, 2017

Conversation

elfey
Copy link
Contributor

@elfey elfey commented Jan 27, 2017

This PR adds support for dot notation in both the usernameField and passwordField.

Solves #7

@elfey elfey mentioned this pull request Jan 27, 2017
@@ -95,32 +97,47 @@ describe('Verifier', () => {
expect(result).to.deep.equal(user);
});
});

it('allows dot notation for password field', () => {
const oldPasswordField = verifier.options.passwordField;
Copy link
Member

Choose a reason for hiding this comment

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

I just realized that we don't need to store and reset oldPasswordField since everything will be reset before every test.

$limit: 1
};

// set usernameField in query, accepting dot notation
set(query, this.options.usernameField, username);
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering what the default query for nested properties should be since it depends on the database. E.g. Mongodb uses the actual dot separated string { query: { 'username.field': 'test' } } instead of a nested object.

Not sure how SQL databases (Sequelize, Knex) do do it and some (Memory, LocalStorage) do not support nested at all. I think the safest bet might be to use the original way of setting it ([this.options.usernameField]: username) so it works at least with MongoDB (and for everything else you have to write your own verifier). Thoughts?

Copy link
Contributor Author

@elfey elfey Jan 27, 2017

Choose a reason for hiding this comment

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

Hm, I see. I have no issue with writing custom verifier for applications; that makes sense because every application has different needs and requirements.

I was originally thinking that the user would use dot notation for usernameField if he knew his database supported it, and if he knows it doesn't support dot notation he would just write the root field.

That still doesn't solve the problem of MongoDB not using nested objects.

And then of course there's the error handling (letting the user know that the usernameField he specified is incorrect for his specific database, as opposed to no user being found) which could get messy pretty quickly.

So a custom verifier sounds like the way to go.

I'm not sure if this is being done yet, but maybe in the future we could attach some data to the service that specifies which type of database driver the user has attached to the service. That would allow us to perform different operations inside of feathers packages based on those values.

@daffl
Copy link
Member

daffl commented Jan 27, 2017

This is great! Two comments and once we figure those out we can get it in.

elfey and others added 2 commits January 27, 2017 10:59
Storing the password and resetting the object wasn't needed because Mocha gives us a clean slate for every test it performs
Lots of edge-cases that weren't being handled with this implementation. Better to remove these changes for now and come back to this later.

For now, if you need dot notation for your usernameField you're better off writing a custom verifier function.
@daffl daffl merged commit 400b1dc into feathersjs-ecosystem:master Jan 27, 2017
@daffl
Copy link
Member

daffl commented Jan 27, 2017

Released as v0.3.3, thank you!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants