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

Enforce "Uniqueness" of fields #176

Closed
jesstelford opened this issue Aug 2, 2018 · 6 comments
Closed

Enforce "Uniqueness" of fields #176

jesstelford opened this issue Aug 2, 2018 · 6 comments
Assignees

Comments

@jesstelford
Copy link
Contributor

eg;

keystone.createList('User', {
  fields: {
    email: { type: Text, unique: true },
  },
});

Which enforces only one User per email address.

@jesstelford
Copy link
Contributor Author

jesstelford commented Sep 26, 2018

@jesstelford jesstelford self-assigned this Sep 26, 2018
@JedWatson
Copy link
Member

Some notes:

async list creation

  • We're going to make model (List) creation async
  • Let's do the second (queue up promises with createList()) -- could we also expose the promise so that client code doesn't have to wait for it, but can if for some reason it wants to?
  • Let's pull in @molomby to line this up with text search index creation?

unique indexes in mongoose

  • However we specify the unique option on fields, it should (by default / without more granular configuration) set up the unique constraint in the database
  • If that fails, connect should fail and tell the developer it's up to them to recover the state of the system
  • What if we want to specify custom rules or queries, or a "generate a unique value" type thing?
  • We had something in Keystone 4 that allowed up to 10 attempts to create something unique, useful for things like slug fields where you generate a unique url based on a title, etc

Maybe that last thing is a field type (generatedKey or something?)

validation

  • This means we need a good spec on how fields should support validation, how this works with hooks, and how the errors are surfaced in the GraphQL API

@jesstelford
Copy link
Contributor Author

Work is underway in the unique-mongo-fields branch.

@molomby
Copy link
Member

molomby commented Oct 3, 2018

Ah, glad someone's on top of this 👌🏼

Keystone 4 handled this stuff quite badly. Indexes were created in the background and we listened to the index and index-single-done events, emitted by the Model. These fired when indexing completed or failed (Mongoose 4.x) but we didn't really handle the errors. As a result attempting to create a unique index on invalid data would fail (almost) silently. 😞

Various comments...

Backgrounding

Index creation can easily take minutes or longer; I agree it should be async and run in the background by default. For simple deployments (eg. to Heroku or similar), adding indexes like this will degrade performance; awaiting the process may cause an extended outage.

Also, this is only relevant to Mongoose, yes? For relational DBs this stuff will be handled by the (yet to be defined) migration framework (#299) which is a whole other problem.

@JedWatson I'm a bit confused by your comment..

However we specify the unique option on fields, it should (by default / without more granular configuration) set up the unique constraint in the database
If that fails, connect should fail and tell the developer it's up to them to recover the state of the system

I don't think this works. These errors during the indexing operation so, assuming indexing is in the background by default, connect may have succeeded minutes earlier. Am I misunderstanding?

Vestigial Indexes

Since there's not really any schema management with Mongo, databases tend to accumulate indexes. Eg. an index: true is added to a field in Keystone then later removed (from the code) but the index itself is never dropped from the production DB.

It's just something to be aware of. On previous projects we occasionally just dropped all indexes and let Mongoose ensureIndex them back in existence. Bit of a "nuclear approach" but did the trick.

Full-text indexes

At this level, full-text index creation isn't fundamentally different. We should handle them the same as other indexes.

Other Options

In addition to unique support I'd love to see Keystone support partial indexes. If we chose to, we could imply this from the required flag. So, for example:

keystone.createList('User', {
  fields: {
    // The user's tax file number isn't required but, if it's supplied, must be unique
    taxFileNumber: { type: Text, required: false, unique: true },

    // ..
  }
};

This is something that needs to be explicitly allowed. Without partial index support the above example will prevent more than one null value being inserted.

Also, for adapters/DBs that support them, conditional indexes are great. They can, for example, be especially handy for simple, DB-based queues.

"Make Unique"

What if we want to specify custom rules or queries, or a "generate a unique value" type thing? We had something in Keystone 4 that allowed up to 10 attempts to create something unique, useful for things like slug fields where you generate a unique url based on a title, etc. Maybe [this] is a field type (generatedKey or something?)

This is handy and certainly related to unique indexes but want to see this handled separately. I'm strongly in favour of Implementing this functionality as different field type (or something) rather than tacking it into the indexing config of a Text field.

Validation

This means we need a good spec on how fields should support validation

We touch on this in #244. It'd be awesome to get some clarity around the validation return format; we're going to want to expose this to the hooks. Eg. a custom pre-save hook should be able to do it's own funky validation then return errors that hook into the validation presentation logic in the admin UI.

@dcousens
Copy link
Member

It appears to have been renamed to isUnique, but is this enforced at the moment?

@dcousens
Copy link
Member

From reading https://github.com/keystonejs/keystone-5/issues/433 , this is completed.
Re-open if not.

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

No branches or pull requests

4 participants