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

feature: supporting "composite rules" #5

Closed
nlac opened this issue Mar 18, 2015 · 17 comments
Closed

feature: supporting "composite rules" #5

nlac opened this issue Mar 18, 2015 · 17 comments

Comments

@nlac
Copy link

nlac commented Mar 18, 2015

Hi,
there are cases when a model should be validated against two or more attributes together.
A simple example, there's a model "budget", attributes "project1", "project2" (meaning the money to be spent for two projects), in a form the user should fill project1, project2 and the sum of the values cannot be more than 1000.

Do you have some suggestion how to handle it with js-data/js-data-schema? If it isn't possible atm, could you consider it as a feature request?

@jmdobry
Copy link
Member

jmdobry commented Mar 18, 2015

This might be possible using validate and asynchronous rules. But it would be kind of clunky.

A possible new feature would be the ability to specify dependent fields in rules, so you can examine more than one attribute during a rule check.

@nlac
Copy link
Author

nlac commented Mar 18, 2015

Hi,
as i can see, it isn't possible atm, neither in a clunky way:( problem is, we cannot let a newly created rule know about other attribute values of the current model instance to be validated (looking at Schemator.defineRule). If i'm wrong pls let me know.
Thank you for considering it as an enhancement!

@jmdobry
Copy link
Member

jmdobry commented Mar 18, 2015

A quick solution would be to invoke the rule function with the model instance as the context (this). That way you can access whatever you want on the model instance in your function.

@nlac
Copy link
Author

nlac commented Mar 18, 2015

Ok, having the model instance as context ("this") in the rule function is a great idea, that indeed would help to implement composite rules. However i really tried but failed to understand how could i use the quick solution above with the current version..
Rule functions are invoked by the framework at certain points of the lifecycle or by calling the validate() fn of a schema, not explicitly "by hand" per rules.. so i cannot set their context every time they're called, to the actual model instance - that is different every time. Or did you mean, this quick solution is a direction to implement this enhancement into the framework?
Sorry if i look dumb:) actually i started with js-data today.

@nlac
Copy link
Author

nlac commented Mar 18, 2015

one thing, i forgot to make it clear that i use js-data-schema implicitly through js-data (like many others i guess:-).. so some things i wrote (lifecycle, model instances) are meant in the context of js-data.

@jmdobry
Copy link
Member

jmdobry commented Mar 18, 2015

Nah you're fine.

What I meant in my last comment was that I could modify js-data-schema to invoke the rule function in the context of the model instance. So, this would be a new feature.

@nlac
Copy link
Author

nlac commented Mar 18, 2015

Oh clear! Thanks.

@nlac
Copy link
Author

nlac commented Mar 23, 2015

It seems to be a workaround for now, but would be better to access the model in the rules as described above..

DS.defineResource({
    name: "Budget", 
    schema: {
        project1: {},
        project2: {},
        sum: {
            max: 1000
        }
    },
    computed: {
        sum: ['project1', 'project2', function(project1, project2) {
            return Number(project1) + Number(project2);
        }]
    }
});

@gtarcea
Copy link
Contributor

gtarcea commented Mar 31, 2015

I'd like to add my vote for this feature. I have some complex relationships between different parts of the schema that I need to validate. Along with this I could really use an extension to the rules to allow passing in an object so I can send additional parameters to a rules extension. Right now I work around this by sending in a string with colons separating the different arguments.

Ideally I would like to say something like the following:

schema.defineSchema('Samples', {
...
measurement_name: {
type: 'string',
minLength: 1,
mustExist: {table: 'measurements', index: 'name', self: true}
}
});

With this definition my mustExist rule gets called with an object as its parameter, the self parameter in the object gets filled in with the actual object being tested (so I have access to all the values).

I've actually been playing around with passing in an object as an argument to the rule. With some small modifications to the code it seems to work. The object passed in is a little wonky, but workable. It looks as follows:
{table: {type: 'measurements'}, index: {type: 'name'}} (note: I haven't tried implementing the self feature as this was just an experiment).

I can send in a pull request if something like this is acceptable.

@jmdobry
Copy link
Member

jmdobry commented Mar 31, 2015

When you define a schema, the "type" rule can be just a string as a shorthand:

{
  name: 'string'
}

is the same as

{
  name: {
    type: 'string'
  }
}

This is why you got {table: {type: 'measurements'}, index: {type: 'name'}}.

You can define schemata of arbitrary nested depth, which requires recursion to parse. The recursion needs to know when to go deeper vs when it's gone deep enough and needs to be interpreting its current level as rules for the parent key. Right now the implementation decides to recurse deeper when it encounters an object. So when you pass an object to a rule, the rule is no longer a rule, it has become a deeper part of your schema. In your example, by setting the value of mustExist to an object, measurement_name, type, minLength, and mustExist all became part of your schema, no longer interpreted as rules. The new mustExist part of your schema now has rules table, index, and self.

This is why you can't use an object as the rule options, because to the parser this object is indistinguishable from a deeper part of your schema. If you can think of a way around this that doesn't complicate the API, I'm all ears.

@gtarcea
Copy link
Contributor

gtarcea commented Mar 31, 2015

Indeed, I noticed that in the code. We could look for a special key to stop the recursion. For example, if the object contained a top level key that was equal to the rule name then we wouldn't recurse further on that entry. As a concrete example:

schema.defineSchema('Samples', {
   name: {  
        type: 'string'
        nullable: true
   },
   measurement: {
      type: 'string',
      mustExist: {mustExist: {index: 'myindex', key: 'thekey'}}  // don't recurse rule name in object
   }
});

@jmdobry
Copy link
Member

jmdobry commented Mar 31, 2015

This would eliminate some schema formats, and I think you would really want to create a nested object like that, just to pass some options. What's wrong with using an array? Perhaps we could make it so the rule function is invoked with the array values as individual arguments?

schemator.defineRule('mustExist', function (x, index, key) {
  // ...
});
schemator.defineSchema('Samples', {
   name: {  
        type: 'string'
        nullable: true
   },
   measurement: {
      type: 'string',
      mustExist: ['myindex', 'thekey'] // index, key
   }
});

@gtarcea
Copy link
Contributor

gtarcea commented Mar 31, 2015

An array might work. How would passing in the object itself (so the rule has access to all the other attributes) work? My original thought was to introduce a keyword such as self. So, then the argument would look like:

measurement: {
   type: 'string',
  mustExist: {mustExist: {index: 'myindex', key:'mykey', self: true}}
}

What schema formats would this prevent? Since its a custom rule and the user chose to use that rule, then where that rule is used they would know that the schema definition parsing stops there.

@jmdobry
Copy link
Member

jmdobry commented Mar 31, 2015

I would like to make this work without adding more keywords or formats.

What about changing it so that the rule function is invoked with the object under tests as the context? So you could do this in the rule function to access the whole object under test.

@gtarcea
Copy link
Contributor

gtarcea commented Mar 31, 2015

Calling the rules function with the object in the this context would be great. I could then just keep my strings separated by colons parameter for my custom rules. That would meet all my needs. Would you like me to do this and send you a pull request? Its something I could really use now.

@jmdobry
Copy link
Member

jmdobry commented Mar 31, 2015

@gtarcea Rule functions are now called in the context of the object under test.

@gtarcea
Copy link
Contributor

gtarcea commented Apr 2, 2015

Just wanted to say thank you. I've started testing using this, and this allows me to remove an ugly workaround.

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

No branches or pull requests

3 participants