Feature : options.validate integration with `add` #165

mbad0la · 2017-08-03T16:29:27Z

This PR is in reference of #101

mbad0la · 2017-08-03T16:32:21Z

Hey @gr2m
I've implemented implicit validation for addOne method. Can you review it and let me know whether I continue to do something similar, or different for other API methods?

mbad0la · 2017-08-03T16:44:24Z

lib/helpers/add-one.js


+  .then(function () {
+    return internals.put(state, internals.addTimestamps(doc))


Btw, why isn't internals API like internals.put(state, doc)?
Because internals.addTimestamps is accessible inside the definition of internals.put as this.addTimestamps. Am I wrong about this?

gr2m · 2017-08-03T23:34:46Z

Just a quick note that I won’t be able to look into this before next week :(

gr2m · 2017-08-14T03:15:16Z

index.js

@@ -33,6 +33,7 @@ function Store (dbName, options) {
    dbName: dbName,
    PouchDB: options.PouchDB,
    emitter: emitter,
+    validate: options.validate || function () { return true },


we could make this a simple no-op: function () {}

or maybe we should leave it undefined, see my comment below

gr2m · 2017-08-14T03:18:33Z

lib/validate.js

+var Promise = require('lie')
+
+function validate (state, doc) {
+  var self = this


If we would leave state.validate unset if the options wasn’t set in the constructor, then we could check if it is set on this line and if it’s not directly return Promise.resolve()

gr2m · 2017-08-14T03:20:11Z

lib/validate.js

+      reject(error)
+    }
+
+    resolve(doc)


I think what I would do is this:

return Promise.resolve() .then(function () { return state.validate(doc) }) .catch(function (error) { error.name = 'ValidationError' throw error })

This won't take into account whether the validation was successful or not

It would have worked if the validation was supposed to be triggered manually, but we are coupling it with CRUD methods isn't it?

sorry I don’t quite understand?

The validate method can, but does not need to be asynchronous. If it throws or returns a rejected promise, the validation will fail, otherwise it will succeed.

wether or not the validate method in your app sends requests to the server (is that what you mean with CRUD methods), is totally up to you

gr2m · 2017-08-14T03:20:46Z

lib/validate.js

+    var result
+
+    try {
+      result = state.validate.call(self, doc)


I don’t think you need need to bind validate to self here, we try to avoid using this altogether in our codebase

mbad0la · 2017-08-25T12:58:22Z

Hey @gr2m !
I've finished adding some code hooking up validation to add method. Can you review it?

gr2m

hey just a few comments regarding tests, could you have a look at these? I feel bad because the PR is great and it’s going for quite long, if you want I can also merge it as is and we add more tests later?

gr2m · 2017-08-25T16:39:23Z

tests/integration/add.js

+  var store = new Store(name, {
+    PouchDB: PouchDB,
+    remote: 'remote-' + name,
+    validate: function () { throw new Error() }


can you pass in an error message here

Will do. But, how does it impact our tests?

you can check if the error message you throw arrives in the .catch(error) {}, see my comment below: #165 (comment)

gr2m · 2017-08-25T16:39:32Z

tests/integration/add.js

+    t.fail('Expecting ValidationError')
+  })
+  .catch(function (error) {
+    t.is(error.name, 'ValidationError')


can you check for the message thrown above?

message would be undefined or an empty string (don't know for sure) in this implementation as the mock validate method throws new Error()

gr2m · 2017-08-25T16:41:07Z

tests/integration/add.js

+  }, {
+    _id: 'foo',
+    foo: 'baz'
+  }])


I think we can simplify the test here and only create one document.

But it would be interesting to see what happens when we try creating two documents at once, one passes validation and one fails. Could you add a test for that maybe?

This test is specifically for multiple additions.
As for your question, my code will not add any doc unless all the docs pass validation. Let me know if you'd want that behaviour to change?

mbad0la · 2017-08-25T17:08:32Z

@gr2m I'm okay with giving this PR time. No reason to hurry and merge code :)
What I think would be better would be to divide this task into multiple PRs, hooking up the validation on every other DB operation.
This PR can be for add. What'd you say to that?

gr2m · 2017-08-25T21:57:26Z

Sounds like a plan to me :) very good!

mbad0la · 2017-08-26T07:39:59Z

The current status of the PR has hooked-up validation flow with document addition, with the following behaviour :

As long as the validation method doesn't throw an error, it is considered a successful validation no matter what value it returns
For add-many case, if even a single document fails validation, none of the docs will be processed for addition

I believe the tests I have added/modified are good enough to the best of my understanding. Do let me know if you'd like to change some behaviour of this functionality @gr2m ?

gr2m · 2017-08-26T19:42:09Z

As long as the validation method doesn't throw an error, it is considered a successful validation no matter what value it returns

Another thing I missed until now, I’m so sorry. Can you add a test where the validation method returns a rejected Promise? It should fail in that case, too.

If it rejects without an error message, we should maybe add a default message? What do you think?

mbad0la · 2017-08-27T04:55:26Z

A rejected Promise will be caught by validate.js code. But the catch needs a little bit more fine tuning if we need to handle Promise rejection.

This is because a Promise can reject with any value (and not just an Error object). It makes much more sense to keep our code resilient to such cases, hence, I'll be taking the following steps in our catch method of validate.js :

Check if the argument passed to the catch method is an Error instance
If it's not, create an Error object
Coerce the name of the error as ValidationError
If error.message is not present
- If the argument passed to catch was an error, assign error.message as your doc(s) failed validation
- If the argument passed to catch wasn't an error, assign error.message as check error value for more details and assign error.value as the argument passed to our catch
Throw the error

So, for example

// someObject is a doc which will fail validation
store.add(someObject)

.then(someFunction)
.catch(function (error) {
  // if our validation rejected with value 'false'
  console.log(error.name) // ValidationError
  console.log(error.message) // check error value for more details
  console.log(`validation evaluated as ${error.value}`) // validation evaluated as false
})

Does this make sense?

gr2m · 2017-08-27T06:24:05Z

yes, that all makes sense 👍

Maybe one addition we can consider to

If the argument passed to catch wasn't an error, assign error.message as check error value for more details and assign error.value as the argument passed to our catch

if the passed argument is a string, then set error.message from to the passed argument

mbad0la · 2017-08-27T06:36:03Z

I was intending to do the same, but got side-tracked. Thanks for catching that 😛

gr2m · 2017-08-27T10:41:45Z

lib/validate.js

@@ -13,7 +13,24 @@ function validate (state, doc) {
    return state.validate(doc)
  })
  .catch(function (error) {
-    error.name = 'ValidationError'
-    throw error
+    var err = new Error()


what do you think about renaming the passed argument from error to rejectValue and var err to var error? I try to avoid abbreviations as much as possible as a general rule

Isn't it misleading to make the argument name rejectValue, considering it can be an error throw directly as well? Although, I can't think of any other name better suited to it 😕

technically a thrown error is captured by a promise and turned into the value that the promise rejects with :)

Dayum. This task has been the best on learning how Promises work 😛

gr2m · 2017-08-27T18:48:03Z

lib/validate.js

      } else {
-        err.message = error.message
+        error.message = rejectValue.message
      }
    }


I would sup around the if / else statements. Try avoiding negations. E.g. do if (rejectValue instanceof Error) { instead of if (!(rejectValue instanceof Error)) { and if (rejectValue.message) { instead of if (!rejectValue.message) {

I was also thinking that maybe people will throw errors with custom data on it, e.g. think a request error that fails with a .status or .statusCode property. These would get lost in this code.

What we could do is that if rejectValue is instanceof Error, we can directly edit that errors .name and defaults its .message property to your doc(s) failed validation?

🤔

I did some local edits and turns out the error we throw is still represented as Error: <message> rather than ValidationError: <message>
Now, I was hoping that it would work as I was avoiding creating a custom Error as it requires the use of this.
I know hoodie tries to avoid using this in the code, so what do you propose we do about this?

This is odd 🤔 It should ™️ be possible to change an error’s name:

const error = new Error('test') error.name = 'ValidationError' error.toString() // ValidationError: test

I don’t see how we would need to use this here? Sorry I must miss something obvious :(

Try throwing this error. You'll see the difference :)

wow I didn’t know you are right

const error = new Error('test') error.name = 'ValidationError' throw error // Unncaught Error: test

But try this

error = new Error('test') error.name = 'ValidationError' Promise.resolve() .then(() => {throw error}) .catch((error) => console.log(error.toString())) // logs ValidationError: test

So my guess is it should work?

Need a slight tweak to what you propose...

error = new Error('test') error.name = 'ValidationError' Promise.resolve() .then(() => {throw error}) .catch((error) => { console.error(error.toString()) return Promise.reject() })

Let me know if we can finalize this, @gr2m

My code was just an example to showcase that the custom error name works. I think we should be good now, if you could finalize it and test the custom error name in a test, that’d be great 👍

gr2m · 2017-08-27T18:48:57Z

lib/validate.js

-      if (!error.message) {
-        err.message = 'your doc(s) failed validation'
+      if (!rejectValue.message) {
+        error.message = 'your doc(s) failed validation'


I would phrase the default error message without using a pronoun. Maybe something like document validation failed

@mbad0la can you make that change?

Sure, I'm just waiting on us to resolve how to go about Error creation/mapping. This is pretty straight-forward to fix.

mbad0la · 2017-09-10T03:48:04Z

Hey @gr2m
I've made some changes. Please have a look at them. Things should be easier for other DB operations now that we have finalized on the validate logic 🙂

gr2m · 2017-09-10T18:35:15Z

tests/unit/add-one-test.js

+test('add-one validation fails with custom error', function (t) {
+  t.plan(4)
+
+  let customError = new Error('custom error message')


we still do only use var right now and don’t use any other es2016 features at this point

that’s why the build is failing: https://travis-ci.org/hoodiehq/hoodie-store-client/jobs/273757512#L682

Writing let has become reflexive for me 😅
Sorry about that. Can't we configure standard for style check regarding usage of ES6 variables?

unfortunately not, that is one downside of standard. There were some discussions to do things like standard-node4 etc but I don’t think it went anywhere. So for now, we have to catch these in our CIs and reviews :D

Thank god you only have to look out for ES6 specifics. Reviews are tough.
On that note, thanks for being patient with this PR, I'll fix this up today :)

gr2m · 2017-09-11T16:35:02Z

I think this is good to go? Could you also update the README: add the validate option to the constructor section

mbad0la · 2017-09-11T20:02:23Z

I still feel console.log(error.toString()) is a bit sketchy, but we'll see about that later on. It might just go well. Other than that, I think I'm done.

gr2m · 2017-09-11T21:22:33Z

README.md

@@ -98,6 +98,7 @@ new Store(dbName, options)
 | **`options.remote`** | Object | PouchDB instance | Yes (ignores `remoteBaseUrl` from [Store.defaults](#storedefaults))
 | **`options.remote`** | Promise | Resolves to either string or PouchDB instance | see above
 | **`options.PouchDB`** | Constructor | [PouchDB custom builds](https://pouchdb.com/custom.html) | Yes (unless preset using [Store.defaults](#storedefaults)))
+| **`options.validate`** | Function | Validation function to execute before DB operations | No


maybe mention that it can return a promise for async validations?

gr2m

👏👏👏

Experiment for options.validate

085e963

mbad0la added in progress Type: Feature labels Aug 3, 2017

mbad0la commented Aug 3, 2017

View reviewed changes

gr2m requested changes Aug 14, 2017

View reviewed changes

Hooking up add-many to validation

e438c99

gr2m requested changes Aug 25, 2017

View reviewed changes

Adding more checks to validation hookup with add

9f2d5a3

Modifications to handle promise rejection in validate

0cdac50

mbad0la added ready and removed in progress labels Aug 27, 2017

mbad0la self-assigned this Aug 27, 2017

gr2m reviewed Aug 27, 2017

View reviewed changes

mbad0la changed the title ~~Feature : options.validate [WIP]~~ Feature : options.validate integration with add Aug 27, 2017

Semantic changes

91d7d69

gr2m reviewed Aug 27, 2017

View reviewed changes

mbad0la added in progress and removed ready labels Aug 28, 2017

Map custom properties from reject errors

4d53e15

mbad0la added ready and removed in progress labels Sep 10, 2017

gr2m reviewed Sep 10, 2017

View reviewed changes

Update add-one-test.js

dc10961

Add validate option for constructor in README

a4b35c1

gr2m reviewed Sep 11, 2017

View reviewed changes

Update README.md

ad903d4

gr2m approved these changes Sep 12, 2017

View reviewed changes

gr2m merged commit 6663e78 into master Sep 12, 2017

gr2m mentioned this pull request Feb 24, 2018

options.validate #101

Closed


		.then(function () {
		return internals.put(state, internals.addTimestamps(doc))

Feature : options.validate integration with add #165

Feature : options.validate integration with add #165

Conversation

mbad0la commented Aug 3, 2017

mbad0la commented Aug 3, 2017 • edited

Choose a reason for hiding this comment

gr2m commented Aug 3, 2017

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

Choose a reason for hiding this comment

mbad0la commented Aug 25, 2017

gr2m 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

mbad0la Aug 25, 2017 • edited

Choose a reason for hiding this comment

mbad0la commented Aug 25, 2017

gr2m commented Aug 25, 2017

mbad0la commented Aug 26, 2017

gr2m commented Aug 26, 2017

mbad0la commented Aug 27, 2017

gr2m commented Aug 27, 2017

mbad0la commented Aug 27, 2017

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

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

Choose a reason for hiding this comment

mbad0la commented Sep 10, 2017

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

gr2m commented Sep 11, 2017

mbad0la commented Sep 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gr2m left a comment

Choose a reason for hiding this comment

Feature : options.validate integration with `add` #165

Feature : options.validate integration with `add` #165

mbad0la commented Aug 3, 2017 •

edited

mbad0la Aug 25, 2017 •

edited