schema: AnyOf FieldComparator implementation + #185 #216

smyrman · 2018-12-11T00:46:40Z

This PR groups a few commits that does the following things in the schema package:

Move the tests of all the FieldComparator implementations to test only the public interface.
Extends comparability to include schema.AnyOf.
Improve schema.AnyOf errors.
Resolve schema: Let AnyOf/AllOf implement the FieldGetter interface #185 (let AnyOf / AllOf implement the FieldGetter interface).

Copy of commit messages (latest first):

commit 1d4ba96 (origin/smyrman/anyof-cmp)
Author: Sindre Røkenes Myren sindre@searis.no
Date: Tue Dec 11 22:08:15 2018 +0100

travis: fix go modules bug

Travis is running pre-scripts to go get everything without go modules
enabled. Moving environment variable to `env` section.

There was a typo in the scripts section when setting GO111MODULE.

commit 7ed6b21
Author: Sindre Røkenes Myren sindre@searis.no
Date: Tue Dec 11 01:40:30 2018 +0100

schema: Allow AnyOf to implement FieldComparator

This changes the interface of FieldComparator so that it's possible for
FieldValidator types to determine, either run-time or compile-time, if
the validator is comparable.

AnyOf has been modified to Allow comparisons if one of it's
sub-validators allows it.

Changes the comparable types Float, Integer, AnyOf and Time to test only
the public interface; testing the private interface gives little value,
as it should be allowed to change without test updates.

commit 749c6a3
Author: Sindre Røkenes Myren sindre@searis.no
Date: Tue Dec 11 01:30:47 2018 +0100

schema: Let AnyOf/AllOf implement FieldGetter

Lets AnyOf and AllOf implement the FieldGetter interface.

commit 52c1b21
Author: Sindre Røkenes Myren sindre@searis.no
Date: Tue Dec 11 01:29:38 2018 +0100

schema: better errors for AnyOf/AllOf + cleanups

AnyOf and AllOf have received better error messages and doc-strings.
They have also been changed so that they will accept any value if there
are no members.

Clean up mergeFieldErrors to be easer to read; Go allows appending to
nil slice and expansion of slices in append, so there is no need for
the if branch and nested for-loop that was there before.

Simplify error handling in Object.Validate slightly. Moves the ErrorMap
type from object.go to errors.go and add some helper methods.

schema/anyof.go

smyrman · 2018-12-11T21:34:30Z

@rs / @Dragomir-Ivanov, this is now ready for review.

Dragomir-Ivanov

Except couple of code comments, LGTM.

schema/anyof_test.go

schema/error.go

smyrman · 2018-12-19T14:04:30Z

Rebased just now. @rs - do you have some time to review this?

rs · 2019-01-28T21:18:58Z

schema/error.go

+// Add adds an error to e and returns a new slice. If err is ErrorSlice, it is
+// extended so that all the elements in err are apppended to e. If err is nil,
+// then no error is appended. Note that calling Add on a nil slice is valid.
+func (e ErrorSlice) Add(err error) ErrorSlice {


As it works the same way as append, why not calling this method Append instead of Add?

rs · 2019-01-28T21:22:10Z

schema/error.go

+}
+
+// Extend copies all errors from err to e.
+func (e ErrorMap) Extend(err ErrorMap) {


Shouldn't this be called Merge?

rs · 2019-01-28T21:25:05Z

schema/error.go

+}
+
+// Tidy returns an un-typed nil if e is empty, and an ErrorMap otherwise.
+func (e ErrorMap) Tidy() error {


That's a strange API. Why do you need that? Couldn't we just expose a Len method and let the caller do what's appropriate?

I agree, it's strange. Will change it. Len could work.

rs · 2019-01-28T21:30:09Z

schema/object.go

-		var errMap ErrorMap
-		errMap = errs
-		return nil, errMap
+	if err := ErrorMap(errs).Tidy(); err != nil {


Per my comment above, I would find it easier to read if it was:

if len(errs) > 0 { return nil, ErrorMap(errs) }

smyrman · 2019-03-05T00:27:08Z

@rs, apologies for my very slow response on this one. I have now updated the PR with your suggested changes.

Dragomir-Ivanov · 2019-03-05T10:54:20Z

Nice work @smyrman . Everything looks good, however I find it strange when we get the first lessFunc for schema.AnyOf. Is this right? It seems that shema.AnyOf is mainly used for shema.AnyOf{shema.Null, shema.XXX}. Can't we just add a field type nullable, and fix this?

smyrman · 2019-03-05T15:33:22Z

We have many use-case for schema.AnyOff besides shema.AnyOf{shema.Null, shema.XXX}, but the place where we need the less function to be implemented is indeed such cases.

Here are some examples of what we have (including some custom FieldValidators):

Validator: &schema.AnyOf{
	&schema.Null{},
	&rsc.Duration{Min: 2 * time.Millisecond, Max: 365 * 24 * time.Hour},
}

Validator: &schema.AnyOf{
	&schema.Null{},
	rsc.UTCTruncTime{Truncate: time.Hour * 24},
}

But even if we removed the Null type and added a Nullable filed (for convenience or as a short-hand), it would still be completely valid to define a schema like this:

Validator: &schema.AnyOf{
	&schema.Interger{Boundaries:&schema.Boundaries{Min:-5,Max:0}},
	&schema.Interger{Boundaries:&schema.Boundaries{Min:5,Max:10}},
}

Weird perhaps, but valid.

I think it would be good to aim for feature completeness in what's supported for AnyOf, AllOf so that they are more useful and diverse.

I am happy to discuss if adding the LessFunc has any unwanted side-effects that would be best to avoid.

AnyOf and AllOf have received better error messages and doc-strings. They have also been changed so that they will accept any value if there are no members. Clean up mergeFieldErrors to be easer to read; Go allows appending to nil slice and expansion of slices in append, so there is no need for the if branch and nested for-loop that was there before. Simplify error handling in Object.Validate slightly. Moves the ErrorMap type from object.go to errors.go and add some helper methods.

Lets AnyOf and AllOf implement the FieldGetter interface.

This changes the interface of FieldComparator so that it's possible for FieldValidator types to determine, either run-time or compile-time, if the validator is comparable. AnyOf has been modified to Allow comparisons if one of it's sub-validators allows it. Changes the comparable types Float, Integer, AnyOf and Time to test only the public interface; testing the private interface gives little value, as it should be allowed to change without test updates.

Travis is running pre-scripts to go get everything without go modules enabled. Moving environment variable to `env` section. There was a typo in the scripts section when setting GO111MODULE.

smyrman · 2019-03-06T09:38:37Z

rebased on master (no new changes)

smyrman · 2019-04-04T23:29:23Z

thx 🙇

smyrman force-pushed the smyrman/anyof-cmp branch 6 times, most recently from 7156c2d to 04e1027 Compare December 11, 2018 14:43

smyrman commented Dec 11, 2018

View reviewed changes

schema/anyof.go Outdated Show resolved Hide resolved

smyrman force-pushed the smyrman/anyof-cmp branch 3 times, most recently from 3ebdaa7 to 1d4ba96 Compare December 11, 2018 21:28

smyrman changed the title ~~WIP: Smyrman/anyof cmp~~ schema: schema.AnyOf FieldComparator + schema.AnyOf/schema.AllOf FieldGetter Dec 11, 2018

smyrman changed the title ~~schema: schema.AnyOf FieldComparator + schema.AnyOf/schema.AllOf FieldGetter~~ schema: AnyOf FieldComparator implementation + #185 Dec 11, 2018

smyrman requested a review from rs December 11, 2018 21:34

Dragomir-Ivanov previously approved these changes Dec 11, 2018

View reviewed changes

schema/anyof_test.go Show resolved Hide resolved

schema/error.go Show resolved Hide resolved

This was referenced Dec 12, 2018

WIP: smyrman/timetrunc #219

Closed

Allow single expression in AND/OR queries #220

Merged

smyrman dismissed Dragomir-Ivanov’s stale review via 7cae1f7 December 19, 2018 14:03

smyrman force-pushed the smyrman/anyof-cmp branch from 1d4ba96 to 7cae1f7 Compare December 19, 2018 14:03

smyrman force-pushed the smyrman/anyof-cmp branch from 7cae1f7 to e8f1840 Compare January 28, 2019 20:07

rs reviewed Jan 28, 2019

View reviewed changes

smyrman force-pushed the smyrman/anyof-cmp branch from e8f1840 to 11ea732 Compare March 5, 2019 00:22

smyrman added 4 commits March 6, 2019 10:34

schema: Let AnyOf/AllOf implement FieldGetter

70b255e

Lets AnyOf and AllOf implement the FieldGetter interface.

travis: fix go modules bug

005ff22

Travis is running pre-scripts to go get everything without go modules enabled. Moving environment variable to `env` section. There was a typo in the scripts section when setting GO111MODULE.

smyrman force-pushed the smyrman/anyof-cmp branch from 11ea732 to 005ff22 Compare March 6, 2019 09:34

rs approved these changes Apr 4, 2019

View reviewed changes

smyrman merged commit 552d179 into master Apr 4, 2019

smyrman deleted the smyrman/anyof-cmp branch April 4, 2019 23:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

schema: AnyOf FieldComparator implementation + #185 #216

schema: AnyOf FieldComparator implementation + #185 #216

smyrman commented Dec 11, 2018 •

edited

Loading

smyrman commented Dec 11, 2018

Dragomir-Ivanov left a comment

smyrman commented Dec 19, 2018

rs Jan 28, 2019

smyrman Mar 5, 2019

rs Jan 28, 2019

smyrman Mar 5, 2019

rs Jan 28, 2019

smyrman Jan 28, 2019

smyrman Mar 5, 2019

rs Jan 28, 2019

smyrman Mar 5, 2019

smyrman commented Mar 5, 2019

Dragomir-Ivanov commented Mar 5, 2019

smyrman commented Mar 5, 2019 •

edited

Loading

smyrman commented Mar 6, 2019

smyrman commented Apr 4, 2019

schema: AnyOf FieldComparator implementation + #185 #216

schema: AnyOf FieldComparator implementation + #185 #216

Conversation

smyrman commented Dec 11, 2018 • edited Loading

Copy of commit messages (latest first):

smyrman commented Dec 11, 2018

Dragomir-Ivanov left a comment

Choose a reason for hiding this comment

smyrman commented Dec 19, 2018

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

smyrman commented Mar 5, 2019

Dragomir-Ivanov commented Mar 5, 2019

smyrman commented Mar 5, 2019 • edited Loading

smyrman commented Mar 6, 2019

smyrman commented Apr 4, 2019

smyrman commented Dec 11, 2018 •

edited

Loading

smyrman commented Mar 5, 2019 •

edited

Loading