Implement predicate type conversion. #204

Dragomir-Ivanov · 2018-10-20T21:06:27Z

Helps with querying for schema.Time and others. It also skips validation for schema.* types during predicate phase of query.
This is just a preview PR, I will fix the tests, when implementation is accepted.

schema/field.go

smyrman

I think the overall approach looks sane -- could you point out the part that worries you @Dragomir-Ivanov?

schema/query/predicate.go

schema/time.go

schema/query/predicate_validator.go

schema/query/predicate.go

Dragomir-Ivanov · 2018-10-24T21:35:27Z

@smyrman My initial concern is:

The name for the validation function ValidateOperator, is not quite right, since we are converting the current Expression and return a new one. So I guess more appropriate name is needed to reflect that.
Predicate is defined as []Expression but we still need to comply with Expression interface, so ValidateOperator is just a noise for Predicate type, i.e. pass-though.

I do like however the way whole operator validation step is separate from other steps, and can be opted-out if needed.

smyrman · 2018-10-25T09:46:46Z

Why is ValidateOperator needed at the Expression andPredicate level? What does it do? Isn't it enough with the new FieldValidator method?

smyrman · 2018-10-25T09:51:16Z

It looks like it validates that the operator is allowed? Can't that be done as part of the normal Validate?

Dragomir-Ivanov · 2018-10-25T10:53:49Z

It looks like it validates that the operator is allowed? Can't that be done as part of the normal Validate?

I am not sure I understand. You want FieldValidator Validate(value interface{}) (interface{}, error) to accept the op as well, and make the check on the spot? But I think Validate is used on other places apart from Query parsing.

Dragomir-Ivanov · 2018-10-25T10:55:42Z

Why is ValidateOperator needed at the Expression andPredicate level? What does it do? Isn't it enough with the new FieldValidator method?

I think your proposal was separate interface OperatorValidator and not adding new method to FieldValidator however I am all for the latter case, and will make the change if you say so.

Also I will try to remove ValidateOperator function from Expression interface and use a global function.

Dragomir-Ivanov · 2018-10-25T10:59:05Z

@smyrman Are we calling interface functions, and struct functions method in Go? I thought method was for OOP principles?

smyrman · 2018-10-25T11:13:23Z

I am not sure I understand. You want FieldValidator Validate(value interface{}) (interface{}, error) to accept the op as well,

no. I mean why does the Predicate and Expression need a new method:

// ValidateOperator TODO
func (e Predicate) ValidateOperator(validator schema.Validator) (Expression, error) {
	return nil, validateOperatorExpressions(e, validator)
}

// Expression is a query or query component that can be matched against a payload.
type Expression interface {
	Match(payload map[string]interface{}) bool
	Validate(validator schema.Validator) error
	ValidateOperator(validator schema.Validator) (Expression, error)
	String() string
}

How does Expression.ValidateOperator differ from Expresssion.Validate? Why is it necessary to return a new expression?

I think your proposal was separate interface OperatorValidator and not adding new method to FieldValidator however I am all for the latter case, and will make the change if you say so.

yes, that was my proposal (to add it as a new interface for the schema package).

smyrman · 2018-10-25T11:57:26Z

@smyrman Are we calling interface functions, and struct functions method in Go? I thought method was for OOP principles?

The concept, in my mind, is the same regarding of language.

Quote: A method is a function with a special receiver argument.

https://tour.golang.org/methods/1

Or, as they say all methods are functions, not all functions are methods.

schema/query/predicate.go

Dragomir-Ivanov · 2018-11-07T12:11:10Z

Hi @smyrman @rs , here is my design proposal for this PR.
Sindre, I didn't quite followed your design on Comparabe interface, but I think this will still be OK.

Lets define 2 new interfaces that can be implemented by schema.FieldValidator

type FieldComparator interface {
	Less(this interface{}, other interface{}) bool
}

type FieldPreparator interface {
        // This can be named Parse as well. I would like to go into schema.FieldValidator interface,
        // so it will be mandatory for all schema types.
	Prepare(value interface{}) (interface{}, error)
}

FieldComarator will be used in the Expression.Match method, and can give custom types rest-layer-mem support. Perfect candidates will be schema.Integer, schema.Float, schema.Time and possibly others.
FieldPreparator will be used in ParsePredicate in order to make sure all query types match the schema defined types. This will not do full type Validate but will only make sure the types are compatible. It will still put as Validate the type in the Expression expected by the DB/Storer.

In order to accomplish the above, I needed to refactor a little bit passed arguments of some functions involved in predicate parsing. I will not go into much details here, because code will explain better.
No change in the public API is made. Please when reviewing this PC, use the Files Changed tab in the GitHub, because last commit undoes much of the previous work.
Thanks for the help, guys!

rs · 2018-11-07T18:47:20Z

Looks good to me.

schema/dependency.go

schema/integer.go

schema/query/predicate_validator.go

smyrman

I think the design looks good overall. I think Prepare on schema needs a better name, otherwise some small comments and advice here and there. God work.

schema/integer.go

schema/query/predicate.go

schema/query/predicate_validator.go

Dragomir-Ivanov · 2018-11-11T18:05:25Z

@smyrman Committed with your suggestions. Didn't implemented Integer field with Float compare value. Lets leave that for later, when we clean things up.
Wanted to add ValidateQuery method to schema.FieldValidator however, schema.Schema also needs to implement method for FieldValidator, and I think that is wrong. We can discuss however.
I did some simple tests, it seems all to work, however when I refactor all broken tests we can have clear picture.

There will be patches for all rest-layer-* Storer implementers, accepting pointer to expressions.

smyrman · 2018-11-12T10:55:32Z

Think this approach looks good @Dragomir-Ivanov 👍 Looking forward to the result :-) I think it's fair not to add ValdateQuery to the FieldValidator interface for now.

Dragomir-Ivanov · 2018-11-12T11:05:27Z

Thanks @smyrman , can you take quick look on the questions above?

smyrman · 2018-11-12T12:26:34Z

Thanks @smyrman , can you take quick look on the questions above?

Tried to answer the ones for unresolved (closed) conversations - Let me know if there are any more questions I have missed.

There will be patches for all rest-layer-* Storer implementers, accepting pointer to expressions.

Do you mean for the type switch?

https://github.com/rs/rest-layer-mongo/blob/8ed09d3750ca8f3efa5367f6c11c9674437d31e0/query.go#L70

Yes, I suppose so... I don't like adding struct pointers to slices (memory gets scattered), but it's fine. We already do slices of pointers for e.g. resource.Items. When I do an overhaul in #77 (eventually) I can have another look at the query design to see if there is anything we can/should do differently there as well.

Dragomir-Ivanov · 2018-11-12T14:30:49Z

I actually made a couple of code comments above on the last commit, since I am not sure how to best Go approach.

smyrman · 2018-11-12T20:50:05Z

I actually made a couple of code comments above on the last commit, since I am not sure how to best Go approach.

Can't see any comments in neither Changed Files nor the last commit itself -- sure they are not with state pending? If they are you need to "submit a review" for me to see them.

Dragomir-Ivanov

Sorry @smyrman, didn't used review functionality before. I thought these are regular code comments. Will not happen again.

schema/dependency.go

schema/query/predicate.go

schema/query/predicate_validator.go

schema/time.go

smyrman

Looks quite good.

README.md

schema/alloff.go

schema/anyof.go

schema/float_test.go

schema/query/predicate_validator.go

smyrman · 2018-11-20T13:17:41Z

schema/query/predicate_validator.go

 				return fmt.Errorf("%s: invalid query expression `%#v': %v", field, v, err)
-			}
-		}
+				}


looks like indenting error -- If there is a way ti enable gofmt or goimports in your editor, it should fix this automatically on save.

I am using VSCode, with the Golang plugin, so I have these enabled. Don't know what happened.

smyrman · 2018-11-20T13:35:01Z

PS! Can you please add a note to the "breaking changes" section of the README with a link to this PR?

Breaking changes is defined as anything that changes the public interface in a way that would cause old code to no longer compile.

smyrman · 2018-11-20T13:36:44Z

Before we do the final merge, please also do a final rebase and squah and write a nice commit message :-)

Dragomir-Ivanov · 2018-11-21T10:50:33Z

@smyrman This is what I recall for the breaking changes part. Do you recall something else?

- PR #204: 
	- Storage drivers need to accept pointer to `Expression` implementer in `query.Predicate`.
	- `filter` parameters in sub-query will be validated for type match.
	- `filter` parameters will be validated for type match only, instead of type & constrains.

Dragomir-Ivanov · 2018-11-21T18:02:17Z

@smyrman Pushed the squashed rebase. When you merge it, I will do additional PRs to fix the DB drivers.

EDIT: It seems that SQLite3 and Google Data Store drivers are rotten. They complain about missing schema.Lookup. Only rest-layer-es is eligible for quick fix.

smyrman · 2018-11-22T08:58:49Z

Yes. Third-party drivers seam out of date.

schema/alloff.go

schema/array.go

smyrman · 2018-11-22T11:05:33Z

Sorry for comming with even more comments now... I sort of look at this in between other things and haven't been able to really spot everything in my first pass through. Thanks a lot for your great work!

Dragomir-Ivanov · 2018-11-22T14:08:53Z

No problem Sindre, I am learning a lot of things during these passes, so let it be as many as needed :)
I have made a new commit addressing your suggestions. Did separate commit so it will be easier for you to review. Then you can make q squash merge or I can squash the PR if you want.

smyrman

LGTM -- @rs, you want to have a final look to see if you can spot something?

smyrman · 2018-11-26T14:43:20Z

@Dragomir-Ivanov can you squash the last fixup commits as well? If there are no more comments, I think this is good for merging and we can deal with any fallout if there is anything we haven't spotted yet.

Checks query parameters if applicable to concrete schema resource types. Doesn't not check type constrains. Checks sub-resource query parameters. Adds suport for custom comparable types, and support for `rest-layer-mem`.

Dragomir-Ivanov · 2018-11-26T14:53:20Z

@smyrman Done.

smyrman requested changes Oct 21, 2018

View reviewed changes

schema/field.go Outdated Show resolved Hide resolved

smyrman reviewed Oct 24, 2018

View reviewed changes

smyrman reviewed Oct 25, 2018

View reviewed changes

schema/query/predicate.go Outdated Show resolved Hide resolved

Dragomir-Ivanov commented Nov 7, 2018

View reviewed changes

schema/dependency.go Outdated Show resolved Hide resolved

Dragomir-Ivanov commented Nov 7, 2018

View reviewed changes

schema/integer.go Show resolved Hide resolved

Dragomir-Ivanov commented Nov 7, 2018

View reviewed changes

schema/query/predicate_validator.go Outdated Show resolved Hide resolved

Dragomir-Ivanov commented Nov 7, 2018

View reviewed changes

schema/query/predicate_validator.go Outdated Show resolved Hide resolved

smyrman reviewed Nov 8, 2018

View reviewed changes

Dragomir-Ivanov commented Nov 12, 2018

View reviewed changes

schema/dependency.go Outdated Show resolved Hide resolved

schema/query/predicate.go Outdated Show resolved Hide resolved

schema/query/predicate_validator.go Outdated Show resolved Hide resolved

schema/time.go Outdated Show resolved Hide resolved

smyrman reviewed Nov 20, 2018

View reviewed changes

smyrman reviewed Nov 22, 2018

View reviewed changes

schema/alloff.go Outdated Show resolved Hide resolved

schema/alloff.go Outdated Show resolved Hide resolved

schema/array.go Outdated Show resolved Hide resolved

schema/array.go Outdated Show resolved Hide resolved

schema/array.go Show resolved Hide resolved

smyrman previously approved these changes Nov 22, 2018

View reviewed changes

Implement predicate type conversion.

e34f9da

Checks query parameters if applicable to concrete schema resource types. Doesn't not check type constrains. Checks sub-resource query parameters. Adds suport for custom comparable types, and support for `rest-layer-mem`.

Dragomir-Ivanov dismissed smyrman’s stale review via e34f9da November 26, 2018 14:51

smyrman approved these changes Nov 26, 2018

View reviewed changes

smyrman merged commit 4e99227 into rs:master Nov 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement predicate type conversion. #204

Implement predicate type conversion. #204

Dragomir-Ivanov commented Oct 20, 2018

smyrman left a comment •

edited

Loading

Dragomir-Ivanov commented Oct 24, 2018 •

edited

Loading

smyrman commented Oct 25, 2018

smyrman commented Oct 25, 2018

Dragomir-Ivanov commented Oct 25, 2018

Dragomir-Ivanov commented Oct 25, 2018 •

edited

Loading

Dragomir-Ivanov commented Oct 25, 2018

smyrman commented Oct 25, 2018 •

edited

Loading

smyrman commented Oct 25, 2018 •

edited

Loading

Dragomir-Ivanov commented Nov 7, 2018

rs commented Nov 7, 2018

smyrman left a comment

Dragomir-Ivanov commented Nov 11, 2018 •

edited

Loading

smyrman commented Nov 12, 2018 •

edited

Loading

Dragomir-Ivanov commented Nov 12, 2018

smyrman commented Nov 12, 2018 •

edited

Loading

Dragomir-Ivanov commented Nov 12, 2018

smyrman commented Nov 12, 2018

Dragomir-Ivanov left a comment

smyrman left a comment

smyrman Nov 20, 2018

Dragomir-Ivanov Nov 21, 2018

smyrman commented Nov 20, 2018

smyrman commented Nov 20, 2018

Dragomir-Ivanov commented Nov 21, 2018 •

edited

Loading

Dragomir-Ivanov commented Nov 21, 2018 •

edited

Loading

smyrman commented Nov 22, 2018

smyrman commented Nov 22, 2018

Dragomir-Ivanov commented Nov 22, 2018

smyrman left a comment

smyrman commented Nov 26, 2018

Dragomir-Ivanov commented Nov 26, 2018

Implement predicate type conversion. #204

Implement predicate type conversion. #204

Conversation

Dragomir-Ivanov commented Oct 20, 2018

smyrman left a comment • edited Loading

Choose a reason for hiding this comment

Dragomir-Ivanov commented Oct 24, 2018 • edited Loading

smyrman commented Oct 25, 2018

smyrman commented Oct 25, 2018

Dragomir-Ivanov commented Oct 25, 2018

Dragomir-Ivanov commented Oct 25, 2018 • edited Loading

Dragomir-Ivanov commented Oct 25, 2018

smyrman commented Oct 25, 2018 • edited Loading

smyrman commented Oct 25, 2018 • edited Loading

Dragomir-Ivanov commented Nov 7, 2018

rs commented Nov 7, 2018

smyrman left a comment

Choose a reason for hiding this comment

Dragomir-Ivanov commented Nov 11, 2018 • edited Loading

smyrman commented Nov 12, 2018 • edited Loading

Dragomir-Ivanov commented Nov 12, 2018

smyrman commented Nov 12, 2018 • edited Loading

Dragomir-Ivanov commented Nov 12, 2018

smyrman commented Nov 12, 2018

Dragomir-Ivanov left a comment

Choose a reason for hiding this comment

smyrman left a comment

Choose a reason for hiding this comment

smyrman Nov 20, 2018

Choose a reason for hiding this comment

Dragomir-Ivanov Nov 21, 2018

Choose a reason for hiding this comment

smyrman commented Nov 20, 2018

smyrman commented Nov 20, 2018

Dragomir-Ivanov commented Nov 21, 2018 • edited Loading

Dragomir-Ivanov commented Nov 21, 2018 • edited Loading

smyrman commented Nov 22, 2018

smyrman commented Nov 22, 2018

Dragomir-Ivanov commented Nov 22, 2018

smyrman left a comment

Choose a reason for hiding this comment

smyrman commented Nov 26, 2018

Dragomir-Ivanov commented Nov 26, 2018

smyrman left a comment •

edited

Loading

Dragomir-Ivanov commented Oct 24, 2018 •

edited

Loading

Dragomir-Ivanov commented Oct 25, 2018 •

edited

Loading

smyrman commented Oct 25, 2018 •

edited

Loading

smyrman commented Oct 25, 2018 •

edited

Loading

Dragomir-Ivanov commented Nov 11, 2018 •

edited

Loading

smyrman commented Nov 12, 2018 •

edited

Loading

smyrman commented Nov 12, 2018 •

edited

Loading

Dragomir-Ivanov commented Nov 21, 2018 •

edited

Loading

Dragomir-Ivanov commented Nov 21, 2018 •

edited

Loading