Allow schema.Reference to validate as normal field #100

smyrman · 2017-03-20T04:41:47Z

Solves issue #57 by allowing schema.Reference to be validated as a normal field. See commit messages for more details!

smyrman · 2017-03-20T05:09:59Z

schema/reference.go

+	return v.rl.ValidateID(v.Path, value)
+}
+
+// CompileReference validates v.Path against rl and stores rl for later use by v.Validate.


Should be just Compile in this implementation example. This is leftover code that was not updated.

smyrman · 2017-03-20T05:12:57Z

schema/reference.go

+}
+
+// CompileReference validates v.Path against rl and stores rl for later use by v.Validate.
+func (v Reference) CompileReference(rl ResourceLookup) error {


Method should be named Compile(rl ResourceLookup)

smyrman · 2017-03-20T05:24:10Z

One of my big question with this design idea, is weather there will be more objects that various FieldValidator's (or Field's) Compile methods needs access to in the future... If no, then this design might be OK.

smyrman · 2017-04-30T11:45:29Z

TL;DR: I will complete this design to facilitate discussion. It's either this or passing context.Context to the Validate method (where the context would contain the same wrapped resource.Index that in effect implements ResourceLookup).

Design discussion: I had been hoping from some comments by now, but I suppose my code was not complete enough to facilitate a full understanding of the intended design here; especially as I have not implemented the part that must wrap resource.Index into an implementation of the ResourceLookup interface before passing that in to the Schema.Compile() method. Anyway, I have done some thinking myself below that I will outline below.

I have one thing that talks in favor of this desing atm., and that is that it makes sense to validate that the linked Reference resource exist at compile time. It can even help users avoid some bugs from reaching production.

I mentioned on #57 that I have considered replacing the ResourceLookup passed to the Compiler method with a context.Context that can optionally contain a ResourceLookup. That however feels really iffy, as the main purpose of context.Context is not to pass arbitrary data to (deeply nested) functions, but to facilitate deadlines and cancelation. And that is something the Compiler functions should not need!

The Validate function however, could serve well as a method where context.Context could be passed in -- as it could actually make sense to allow for deadline management there -- especially if having to do a DB call to finish the validation, as would be the case for Reference.

I will complete the current design to facilitate discussion, but it's probably a viable alternative to consider going for a Valdiate(ctx context.Context) interface.

smyrman · 2017-05-04T19:05:45Z

I am not to far away from pushing and update... almost got there in the weekend, just need to update some remaining tests...

rs · 2017-05-04T23:23:52Z

Sorry if I'm not very responsive. I'll wrap my head around the subject when you send the PR.

smyrman · 2017-05-05T07:08:00Z

No problem @rs, I have been very busy as well, hench the long delay in pushing any more code.

smyrman · 2017-05-05T09:21:11Z

@rs, There is still some tests that needs updating, but now at least the tests within the schema package pass.

At least tests within the schema/encoding and resource packages needs updating.

smyrman · 2017-05-05T11:05:12Z

resource/resource.go

-// Compile the resource graph and report any error
-func (r *Resource) Compile() error {
+// Compile the resource graph and report any error.
+func (r *Resource) Compile(rl schema.ReferenceChecker) error {


sholud be rc for consistency. the rl name is leftover from when it was called ReferenceLookup

smyrman · 2017-05-08T06:13:21Z

UPDATE: jsonschema tests fixed, although I will add better support for JSON Schema encoding of Reference later now that we can actually access the reference validator after Compile is called.

rest package tests are still failing though, with the following:

2017/05/08 07:59:08 Server error: No Storage Defined
--- FAIL: TestHandlerPostListWithReferenceNoRouter (0.00s)
        Error Trace:    method_post_test.go:229
	Error:  	Not equal: 500 (expected)
			        != 422 (actual)
		
        Error Trace:    method_post_test.go:232
	Error:  	Not equal: 500 (expected)
			        != 422 (actual)
		
        Error Trace:    method_post_test.go:233
	Error:  	Not equal: "Router not available in context" (expected)
			        != "Document contains error(s)" (actual)
		
--- FAIL: TestHandlerPostListWithInvalidReference (0.00s)
        Error Trace:    method_post_test.go:255
	Error:  	Not equal: 500 (expected)
			        != 422 (actual)
		
        Error Trace:    method_post_test.go:258
	Error:  	Not equal: 500 (expected)
			        != 422 (actual)
		
        Error Trace:    method_post_test.go:259
	Error:  	Not equal: "Invalid resource reference for field `foo': invalid" (expected)
			        != "Document contains error(s)" (actual)
		
.... more failures like this.

I am not quite sure why, but presumably it's somehow related to issues with the new index.Compile() ?

rs

Looks great.

rs · 2017-05-21T21:13:02Z

resource/index.go

@@ -5,22 +5,22 @@ import (
 	"net/url"
 	"strings"

+	"golang.org/x/net/context"


wrong context?

rs · 2017-05-21T21:18:28Z

schema/reference.go

+	if v := rc.ReferenceChecker(r.Path); v != nil {
+		r.validator = v
+		return nil
+


extra empty line

smyrman · 2017-05-23T04:56:09Z

@rs, thanks for the review, and sorry for my slow update time on this. Pushing a minor update to resolve identified issues, but there is still one major bug that surface in the resource package that needs some debugging. Reproduce by running tests.

If someone with some time on their hans want to try and debug it, feel welcome. Otherwise I will get to it eventually.

smyrman · 2017-06-28T19:12:34Z

Sorry for the the delay in looking into this.

I think most of the errors looks pretty reasonable, since the code generating the error texts / status codes we test against here, is part of what has been removed.

There is however one failing test that is not expected, and that's this one:

--- FAIL: TestHandlerPostListWithSubSchemaReference (0.00s)
        Error Trace:    method_post_test.go:370
	Error:  	Not equal: 201 (expected)
			        != 422 (actual)
		
        Error Trace:    method_post_test.go:371
	Error:  	Object expected to be of type *resource.Item, but was *rest.Error

smyrman · 2017-06-28T21:43:38Z

Right, so the 201 issues are solved by adding an explicit Compile to index. Normaly, index is compiled by rest.NewHandler(index), but that method is not called in these tests. Previously, this tests worked because a schema compilation was not specifically needed for schema.Reference fields, or any of the other fields in this specific schemas.

This leaves the remaining issues with unexpected status codes / error messages when POSTing documents due to changed semantics. These tests also lacks an explicit call to index.Compile(), so when correcting the tests, I think we can write them to be go 1.7 sub-tests, so that there is less code duplication.

@rs, what's your opinion on changing the behaviour for "invalid reference" to all return a 422 (Unprocessable Entity), rather than different type of errors based on what's wrong with the Reference field. The reason that the status code is changing, is that invalid Reference fields now follow the same code pattern as other validation issues.

--- FAIL: TestHandlerPostListWithReferenceNoRouter (0.00s)
        Error Trace:    method_post_test.go:229
	Error:  	Not equal: 500 (expected)
			        != 422 (actual)
		
        Error Trace:    method_post_test.go:232
	Error:  	Not equal: 500 (expected)
			        != 422 (actual)
		
        Error Trace:    method_post_test.go:233
	Error:  	Not equal: "Router not available in context" (expected)
			        != "Document contains error(s)" (actual)
		
--- FAIL: TestHandlerPostListWithInvalidReference (0.00s)
        Error Trace:    method_post_test.go:255
	Error:  	Not equal: 500 (expected)
			        != 422 (actual)
		
        Error Trace:    method_post_test.go:258
	Error:  	Not equal: 500 (expected)
			        != 422 (actual)
		
        Error Trace:    method_post_test.go:259
	Error:  	Not equal: "Invalid resource reference for field `foo': invalid" (expected)
			        != "Document contains error(s)" (actual)
		
--- FAIL: TestHandlerPostListWithReferenceOtherError (0.00s)
        Error Trace:    method_post_test.go:282
	Error:  	Not equal: 500 (expected)
			        != 422 (actual)
		
        Error Trace:    method_post_test.go:285
	Error:  	Not equal: 500 (expected)
			        != 422 (actual)
		
        Error Trace:    method_post_test.go:286
	Error:  	Not equal: "Error fetching resource reference for field `foo': No Storage Defined" (expected)
			        != "Document contains error(s)" (actual)
		
--- FAIL: TestHandlerPostListWithReferenceNotFound (0.00s)
        Error Trace:    method_post_test.go:309
	Error:  	Not equal: 404 (expected)
			        != 422 (actual)
		
        Error Trace:    method_post_test.go:312
	Error:  	Not equal: 404 (expected)
			        != 422 (actual)
		
        Error Trace:    method_post_test.go:313
	Error:  	Not equal: "Resource reference not found for field `foo'" (expected)
			        != "Document contains error(s)" (actual)
		
--- FAIL: TestHandlerPostListWithSubSchemaReferenceNotFound (0.00s)
        Error Trace:    method_post_test.go:407
	Error:  	Not equal: 404 (expected)
			        != 422 (actual)
		
        Error Trace:    method_post_test.go:410
	Error:  	Not equal: 404 (expected)
			        != 422 (actual)
		
        Error Trace:    method_post_test.go:411
	Error:  	Not equal: "Resource reference not found for field `foo'" (expected)
			        != "Document contains error(s)" (actual)

rs · 2017-06-30T05:29:17Z

Yes I think it make sense. And for Go 1.7 sub-test, I'm all for it.

smyrman · 2017-07-07T15:15:05Z

I have set the sub-tests to run in Parallel. This appears to uncover a data race within github.com/rs/rest-layer/rest.DefaultResponseFormatter.FormatError(), but I don't think it's related to this change.

The bug goes away if I remove the parallel execution of the sub-tests. I think the bug was just masked before because we didn't do to many parallel tests...

smyrman · 2017-07-07T15:23:00Z

It's these lines:

func (f DefaultResponseFormatter) FormatError(ctx context.Context, headers http.Header, err error, skipBody bool) (context.Context, interface{}) {
        oldLogger := resource.Logger
        resource.Logger = nil
        defer func() { resource.Logger = oldLogger }()

It think this is pretty ugly:-/ If we really need to do this, we need to use a mutex. What's the benfit of silencing the logger? @rs, do you want to fix it in a separate commit?

This PR should be otherwise good to go in now :-)

rs · 2017-07-07T16:40:15Z

I did that in some tests to remove error output while testing error cases? Did I do that in actual code??

smyrman · 2017-07-07T21:19:48Z

Did I do that in actual code??

Well, I didn't run a git blame on it yet, but someone did... if it's by mistake I can add a separate commit to remove it in this PR so that the tests pass.

And we can of course silence the loggers in test code (or re-route it to write to testing.T).

smyrman · 2017-07-07T21:47:49Z

The Git-blame btw:

4d81026

smyrman · 2017-07-07T21:59:24Z

schema/anyof.go

@@ -25,8 +25,7 @@ func (v AnyOf) Compile(rc ReferenceChecker) error {
 // Validate ensures that at least one sub-validator validates.
 func (v AnyOf) Validate(value interface{}) (interface{}, error) {
 	for _, validator := range v {
-		var err error
-		if value, err = validator.Validate(value); err == nil {
+		if value, err := validator.Validate(value); err == nil {


The initial problem here is that we where overwriting the value functional parameter, so that the next iteration of the for-loop would use the value returned by the previous (failing) validator, which in most cases would be nil due to the returned error.

With this fix, we generate a new (shadowing) value variable for each iteration. This variable is not passed on to the next iteration.

smyrman · 2017-07-08T04:37:11Z

I will rebase one more time to put the bug-fixes in front of the feature, leaving all three commits with passing tests.

DataResponseFormatter was modifying the global response Logger instance in a non-threadsafe way, presumably to supress logging during testing. This code has now been removed.

smyrman · 2017-07-08T04:43:53Z

@rs, ready for final review. The two commits before the feature are (unrelated) bugfixes that where needed for the (updated) tests to pass, hench I recommend a rebase and merge so that they end up as separate commits on master :-)

Note that the commit ordering showed on the PR is a bit weird... It's the one with the green tick that's last. The git log on the branch is correct.

rs · 2017-07-08T07:14:32Z

resource/index.go

-	}
-	return nil
+
+		_, err = rsc.Get(context.TODO(), id)


Why TODO over Background?

From the docs on context.TODO():

Code should use context.TODO when it's unclear which Context to use or it is not yet available (because the surrounding function has not yet been extended to accept a Context parameter).

From the docs on context.Background():

It is typically used by the main function, initialization, and tests, and as the top-level Context for incoming requests.

I feel this case is closer to the description offered for context.TODO() than what's offered by context.Background(). I think if we where to use the right context here, we should get it passed in from somewhere so that when using the rest package, we use the context created for a new request, and not a new context created later.

rs · 2017-07-08T07:14:32Z

schema/anyof.go

 }

 // Validate ensures that at least one sub-validator validates.
 func (v AnyOf) Validate(value interface{}) (interface{}, error) {
 	for _, validator := range v {
-		var err error
-		if value, err = validator.Validate(value); err == nil {
+		if value, err := validator.Validate(value); err == nil {


Good catch.

rs · 2017-07-08T07:14:32Z

schema/field.go

+			if err := c.Compile(rc); err != nil {
+				return fmt.Errorf(": %v", err)
+			}
+		} else if c, ok := f.Validator.(Compiler); ok {


Wouldn't it be simpler to only have the ReferenceCompiler interface and remove the Compiler interface altogether? It is up to the type to use the rc or not.

Having both interfaces available is not clear on what happen if a given type implement both interfaces.

It's not possible for one type to implement both interfaces since method overloading is not legal in Go, and the methods have the same name

Yes you are right. But having a single interface would be simpler and make sure we don’t forget to always check for both interfaces everywhere it’s needed.

The Compiler interface is also used within the resource package, e.g. for the index.

I can check if it's feasible to move the Compiler interface to the resource package, and only use the new ReferenceCompiler interface within the schema package. It's an even bigger change, especially when it commes to updating tests, but should be mostly search/replace.

smyrman · 2017-07-09T17:36:39Z

@rs, I have pushed rename schema.ReferenceCompiler -> Compiler as a new commit for easier review only.

When you are happy with the PR, I would like to do a manual rebase to fold this, as well as any other requested changes into the feature commit :-)

Allows for schema.Reference to be validated in the same way all other field validators are validated. To achive this, the resource.Index will pass on a new schema.ReferenceChecker implementation when calling Compile on each resource. To achive this, schema.Schema and each schema.FieldValidator implementation that may hold a reference, has been changed to implement a new interface schema.ReferenceCompiler in place of the schema.Compiler interface. This is a breaking change to the public API, but simple programs that don't rely on calling Compile on Schema or Resource, are likely not affected.

smyrman · 2017-07-10T12:25:55Z

rebased (squashing the two last commits).

rs · 2017-07-10T15:02:38Z

\o/

smyrman commented Mar 20, 2017

View reviewed changes

smyrman force-pushed the breaking-compile branch from 05b2419 to 6301a8d Compare May 5, 2017 09:16

smyrman commented May 5, 2017

View reviewed changes

smyrman force-pushed the breaking-compile branch from 6301a8d to 765a265 Compare May 8, 2017 06:00

rs reviewed May 21, 2017

View reviewed changes

smyrman force-pushed the breaking-compile branch 2 times, most recently from 0665f2b to 8a671f3 Compare May 23, 2017 04:54

smyrman force-pushed the breaking-compile branch 2 times, most recently from 58160d4 to 2af67f1 Compare May 23, 2017 05:01

smyrman mentioned this pull request Jun 23, 2017

[JSON Schema] Support for schema.Reference #36

Open

smyrman force-pushed the breaking-compile branch from 2af67f1 to 48f25de Compare June 28, 2017 21:28

smyrman force-pushed the breaking-compile branch from 48f25de to 15417ef Compare July 7, 2017 14:59

smyrman changed the title ~~WIP for Issue #57, DO NOT MERGE!~~ Allow schema.Reference to validate as normal field Jul 7, 2017

smyrman force-pushed the breaking-compile branch from 15417ef to d02d93e Compare July 7, 2017 15:27

smyrman commented Jul 7, 2017

View reviewed changes

smyrman added 2 commits July 8, 2017 06:37

Fix incorrect behaviour for AnyOf validator

b54368e

Fix data race in DefalutResponseFormatter

4e1b0c9

DataResponseFormatter was modifying the global response Logger instance in a non-threadsafe way, presumably to supress logging during testing. This code has now been removed.

smyrman force-pushed the breaking-compile branch from 39bb08e to bd137e7 Compare July 8, 2017 04:37

rs reviewed Jul 8, 2017

View reviewed changes

smyrman mentioned this pull request Jul 9, 2017

Move FieldValidator Validate() methods to be be on a pointer value #110

Closed

smyrman force-pushed the breaking-compile branch from ed42f4f to ab343d9 Compare July 9, 2017 17:39

rs approved these changes Jul 10, 2017

View reviewed changes

smyrman force-pushed the breaking-compile branch from ab343d9 to 07df663 Compare July 10, 2017 12:25

smyrman merged commit 5b33327 into rs:master Jul 10, 2017

smyrman deleted the breaking-compile branch July 10, 2017 12:30

smyrman mentioned this pull request Jul 13, 2017

Validate schema.Reference when embedded in nested FieldValidators #57

Closed

Dragomir-Ivanov mentioned this pull request Dec 31, 2018

Array of references #222

Closed

Allow schema.Reference to validate as normal field #100

Allow schema.Reference to validate as normal field #100

Conversation

smyrman commented Mar 20, 2017 • edited Loading

smyrman Mar 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smyrman commented Mar 20, 2017 • edited Loading

smyrman commented Apr 30, 2017

smyrman commented May 4, 2017

rs commented May 4, 2017

smyrman commented May 5, 2017

smyrman commented May 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smyrman commented May 8, 2017 • edited Loading

rs 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

smyrman commented May 23, 2017 • edited Loading

smyrman commented Jun 28, 2017 • edited Loading

smyrman commented Jun 28, 2017 • edited Loading

rs commented Jun 30, 2017

smyrman commented Jul 7, 2017

smyrman commented Jul 7, 2017 • edited Loading

rs commented Jul 7, 2017

smyrman commented Jul 7, 2017 • edited Loading

smyrman commented Jul 7, 2017

Choose a reason for hiding this comment

smyrman commented Jul 8, 2017

smyrman commented Jul 8, 2017 • edited Loading

Choose a reason for hiding this comment

smyrman Jul 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smyrman Jul 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smyrman commented Jul 9, 2017 • edited Loading

smyrman commented Jul 10, 2017 • edited Loading

rs commented Jul 10, 2017

smyrman commented Mar 20, 2017 •

edited

Loading

smyrman Mar 20, 2017 •

edited

Loading

smyrman commented Mar 20, 2017 •

edited

Loading

smyrman commented May 8, 2017 •

edited

Loading

smyrman commented May 23, 2017 •

edited

Loading

smyrman commented Jun 28, 2017 •

edited

Loading

smyrman commented Jun 28, 2017 •

edited

Loading

smyrman commented Jul 7, 2017 •

edited

Loading

smyrman commented Jul 7, 2017 •

edited

Loading

smyrman commented Jul 8, 2017 •

edited

Loading

smyrman Jul 9, 2017 •

edited

Loading

smyrman Jul 8, 2017 •

edited

Loading

smyrman commented Jul 9, 2017 •

edited

Loading

smyrman commented Jul 10, 2017 •

edited

Loading