Fix for field validation on nested sub-resources #231

p7900 · 2019-01-19T13:46:28Z

After @Dragomir-Ivanov added nested sub-resources I found that is some issue when you have multiple sub-resources and schema references, for example:

user = schema.Schema{
  Fields: schema.Fields{
    "id": schema.IDField,
  },
}

ticket = schema.Schema{
  Fields: schema.Fields{
    "id": schema.IDField,
    "user": {
      Required:   true,
      Filterable: true,
      Validator: &schema.Reference{
        Path: "users",
      },
    },
  },
}

message = schema.Schema{
	Fields: schema.Fields{
		"id": schema.IDField,
		"ticket": {
			Required:   true,
			Filterable: true,
			Validator: &schema.Reference{
				Path: "users.tickets",
			},
		},
	},
}

http POST :8080/api/users/[user-id]/tickets/[ticker-id]/messages

Give you error:

{
    "code": 422,
    "issues": {
        "user": [
            "invalid field"
        ]
    },
    "message": "Document contains error(s)"
}

Because base is filled by user, and ticket id, in message you dont have user field so error occured. In this case we will check base field exists in resource fields.

Dragomir-Ivanov · 2019-01-19T14:11:25Z

Hi @pakali , it seems that there are some failing tests. Can you come online at gophers.slack.com, channel #rest-layer so we can discuss in a timely manner.
You should paste also how you are building your resource graph. Did your case works with some more old rest-layer HEAD. Say: ca3afb7adc9e28f4c000023c1706c46b87822970

p7900 · 2019-01-19T15:48:09Z

Hi @Dragomir-Ivanov, tests fails because this commit change logic of application, application after this commit ignore fields that doesnt exists in Schema (so if you pass in tests additional non-exist field as base field then the error doesn't occured and data will be processed by application). This need to be discussed.

You cannot reproduce this problem on older version of rest-layer (like ca3afb7) because there is no nested sub-resources there.

I will join to #slack soon and we will discuss this problem.

Dragomir-Ivanov · 2019-01-19T15:55:14Z

@pakali There were nested sub-resources before, however they are linked with schema.Connection instead of Reference. Can you paste you are builidng your resource graph here.
Note my PR message on #230, on example similar resource graph .

p7900 · 2019-01-19T16:02:53Z

users := index.Bind("users", user, mongo.NewHandler(session, db, "users"), resource.Conf{
  AllowedModes: resource.ReadWrite,
})

users.Bind("tickets", "user", ticket, mongo.NewHandler(session, db, "tickets"), resource.Conf{
  AllowedModes: resource.ReadWrite,
}).Bind("messages", "ticket", message, mongo.NewHandler(session, db, "messages"), resource.Conf{
  AllowedModes: resource.ReadWrite,
})

There is problem only with saving data on endpoint like:
http POST :8080/api/users/[user-id]/tickets/[ticker-id]/messages
Because messages get user id, ticket id, message id and body of message - In message schema we dont have user reference thats why we get this validation error.

Dragomir-Ivanov · 2019-01-19T16:09:29Z

Okay, I can't say for certain, but can you remove your schema.Reference fields from definitions. Rest-layer will put the second parameter of schema.Bind as a filed of schema.Connection type.

user = schema.Schema{
  Fields: schema.Fields{
    "id": schema.IDField,
  },
}

ticket = schema.Schema{
  Fields: schema.Fields{
    "id": schema.IDField,
  },
}

message = schema.Schema{
	Fields: schema.Fields{
		"id": schema.IDField,
	},
}

p7900 · 2019-01-19T16:14:45Z

When I removed references then I get error:
Cannot bind tickets' as sub-resource: field user' does not exist in the sub-resource'

I will read about schema.Connection and give you answer about result of this case.

Dragomir-Ivanov · 2019-01-19T16:17:47Z

Hmm, schema.Connection is implicit, you are NOT supposed to add it in your defintions. Look for the schema.Bind() method, it puts it in the source schema.
I will definitely look more deeply for your problems, but will be later this evening.

p7900 · 2019-01-19T16:24:10Z

I will check this problem with some old version of rest-layer and schema.Connection approach and give you results but tommorow. Im almost sure this will fail too because I suppose every sub-resource give ID to main body of last sub-resource. So if you have just two sub-resources there is no problem with that Ticket get User ID and that how it should works, but if you get 3 nested sub-resources then Message get Ticket and User ID but only Ticket field is on body of Message. If this validation wasnt update last time then schema.Connection should give same error like sub-resources based on references.

Dragomir-Ivanov · 2019-01-19T16:27:16Z

Can you put your Go source somewhere, along with the curl commands you are testing, so I can reproduce locally.
Connection and Resource are like Has-{One,Many} and BelogsTo relations. They are different beasts used for different things. Will explain later.

p7900 · 2019-01-19T16:38:50Z

Sure:

// +build go1.7


var (
	// Define a user resource schema
	user = schema.Schema{
		Fields: schema.Fields{
			"id": {
				Required:   true,
				Filterable: true,
				Validator:  &schema.String{},
			},
		},
	}

	// Define a post resource schema
	ticket = schema.Schema{
		Fields: schema.Fields{
			"id": {
				Required:   true,
				Filterable: true,
				Validator:  &schema.String{},
			},
			"user": {
				Required:   true,
				Filterable: true,
				Validator: &schema.Reference{
					Path: "users",
				},
			},
		},
	}

	message = schema.Schema{
		Fields: schema.Fields{
			"id": {
				Required:   true,
				Filterable: true,
				Validator:  &schema.String{},
			},
			"ticket": {
				Required:   true,
				Filterable: true,
				Validator: &schema.Reference{
					Path: "users.tickets",
				},
			},
		},
	}
)

func main() {
	session, err := mgo.Dial("127.0.0.1:27017")
	db := "test"

	if err != nil {
		panic(err)
	}

	index := resource.NewIndex()

	index.Bind("users", user, mongo.NewHandler(session, db, "users"), resource.Conf{
		AllowedModes: resource.ReadWrite,
	}).Bind("tickets", "user", ticket, mongo.NewHandler(session, db, "tickets"), resource.Conf{
		AllowedModes: resource.ReadWrite,
	}).Bind("messages", "ticket", message, mongo.NewHandler(session, db, "messages"), resource.Conf{
		AllowedModes: resource.ReadWrite,
	})

	api, err := rest.NewHandler(index)
	if err != nil {
		log.Fatal().Msgf("Invalid API configuration: %s", err)
	}

	c := alice.New()

	http.Handle("/api/", c.Then(http.StripPrefix("/api/", api)))

	if err := http.ListenAndServe(":8080", nil); err != nil {
		log.Fatal().Err(err).Msg("")
	}
}

After that:

$ http POST :8080/api/users id=test
HTTP/1.1 201 Created
Content-Length: 13
Content-Location: users/test
Content-Type: application/json
Date: Sat, 19 Jan 2019 16:37:32 GMT
Etag: W/"38c5a156b60082ce31bcd0549f49b6cb"
Last-Modified: Sat, 19 Jan 2019 16:37:32 GMT

{
    "id": "test"
}

$ http POST :8080/api/users/test/tickets id=test
HTTP/1.1 201 Created
Content-Length: 27
Content-Location: users/test/tickets/test
Content-Type: application/json
Date: Sat, 19 Jan 2019 16:37:38 GMT
Etag: W/"82acdab886e7441899b4219166acd897"
Last-Modified: Sat, 19 Jan 2019 16:37:38 GMT

{
    "id": "test",
    "user": "test"
}

$ http POST :8080/api/users/test/tickets/test/messages id=test
HTTP/1.1 422 Unprocessable Entity
Content-Length: 87
Content-Type: application/json
Date: Sat, 19 Jan 2019 16:37:46 GMT

{
    "code": 422,
    "issues": {
        "user": [
            "invalid field"
        ]
    },
    "message": "Document contains error(s)"
}

Dragomir-Ivanov · 2019-01-19T21:55:01Z

Hi @pakali It seems that your code doesn't work even for the commit ID I gave you, before my work was merged. I will try your fix and think about if it is reasonable, or there is some more deeper work to be done. Good catch!

Dragomir-Ivanov · 2019-01-19T22:49:41Z

I got your code working @pakali .
The patched I used over (ca3afb7) is this:
resource_path.go

// Values returns all the key=value pairs defined by the resource path.
func (p ResourcePath) Values() map[string]interface{} {
	// v := map[string]interface{}{}
	// for _, rp := range p {
	// 	if _, found := v[rp.Field]; !found && rp.Value != nil {
	// 		v[rp.Field] = rp.Value
	// 	}
	// }
	// return v

	// Return only the last key=value pair
	v := map[string]interface{}{}
	var lastField string
	var lastValue interface{}
	for _, rp := range p {
		if _, found := v[rp.Field]; !found && rp.Value != nil {
                        // id needs to be there
			if rp.Field == "id" {
				v[rp.Field] = rp.Value
			} else {
				lastField = rp.Field
				lastValue = rp.Value
			}
		}
	}
	if lastValue != nil {
		v[lastField] = lastValue
	}
	return v
}

I don't claim that this is the right way to fix it, however it is a starting point to work on.
@smyrman you input is always welcomed.

@pakali Here is the code:

package main

import (
	"net/http"

	"github.com/justinas/alice"
	"github.com/rs/rest-layer-mem"
	"github.com/rs/rest-layer/resource"
	"github.com/rs/rest-layer/rest"
	"github.com/rs/rest-layer/schema"
	"github.com/rs/zerolog/log"
)

var (
	// Define a user resource schema
	user = schema.Schema{
		Fields: schema.Fields{
			"id": {
				Required:   true,
				Filterable: true,
				Validator:  &schema.String{},
			},
		},
	}

	// Define a post resource schema
	ticket = schema.Schema{
		Fields: schema.Fields{
			"id": {
				Required:   true,
				Filterable: true,
				Validator:  &schema.String{},
			},
			"user": {
				Validator: &schema.Reference{
					Path: "users",
				},
			},
		},
	}

	message = schema.Schema{
		Fields: schema.Fields{
			"id": {
				Required:   true,
				Filterable: true,
				Validator:  &schema.String{},
			},
			"ticket": {
				Validator: &schema.Reference{
					Path: "users.tickets",
				},
			},
		},
	}
)

func main() {
	index := resource.NewIndex()

	index.Bind("users", user, mem.NewHandler(), resource.Conf{
		AllowedModes: resource.ReadWrite,
	}).Bind("tickets", "user", ticket, mem.NewHandler(), resource.Conf{
		AllowedModes: resource.ReadWrite,
	}).Bind("messages", "ticket", message, mem.NewHandler(), resource.Conf{
		AllowedModes: resource.ReadWrite,
	})

	api, err := rest.NewHandler(index)
	if err != nil {
		log.Fatal().Msgf("Invalid API configuration: %s", err)
	}

	c := alice.New()

	http.Handle("/api/", c.Then(http.StripPrefix("/api/", api)))

	if err := http.ListenAndServe(":8080", nil); err != nil {
		log.Fatal().Err(err).Msg("")
	}
}

POST http://localhost:8080/api/users

{
  "id":"test"
}

###
POST http://localhost:8080/api/users/test/tickets

{
  "id":"test"
}

###
POST http://localhost:8080/api/users/test/tickets/test/messages

{
  "id":"test"
}

###

If you checkout my `rest-layer` repo, you will be able to use this query
GET  http://localhost:8080/api/users?fields=tickets{messages}

p7900 · 2019-01-20T20:33:40Z

Thanks for this, both fixes yours and mine working fine, probably your fix is more proper one.

Dragomir-Ivanov · 2019-01-20T22:50:44Z

Yes @pakali , I think the same. But lets wait for some input from the owners of the project, since I don't have much insight in this part of the code.

smyrman · 2019-01-28T10:42:21Z

For triple-nested resources, it was always necessary to add all of the outer keys to the inner resource. I.e.:

message = schema.Schema{
		Fields: schema.Fields{
			"id": {
				Required:   true,
				Filterable: true,
				Validator:  &schema.String{},
			},
			"users": {
				Validator: &schema.Reference{
					Path: "users",
				},
			},
			"ticket": {
				Validator: &schema.Reference{
					Path: "users.tickets",
				},
			},
		},
	}

By setting the fields ReadOnly, you would avoid items being movable via PATCH or PUT. If we are to allow not having the outer resource in the inner resource (e.g. skipping users), it would be nice if it still worked to do so. Thus the solution should presumably add all route values that are in the resource schema (ignoring other keys). I.e. just updating the last field in the path, is not something I would recommend. This may involve updating some unit-test schemas so that the tests pass.

smyrman · 2019-01-28T10:42:41Z

Sorry for a very late reply on this one.

Dragomir-Ivanov · 2019-01-28T11:50:49Z

Sorry for a very late reply on this one.

Thanks Sindre and don't worry.

Thus the solution should presumably add all route values that are in the resource schema

you mean the target resource, i.e. the last one in the url path?

smyrman · 2019-01-28T12:31:55Z

you mean the target resource, i.e. the last one in the url path?

I mean if we do POST /users/:id/tickets/:id/messages,

"ticket" (or which ever field name was used in tickets.Bind(...)) gets set in the message item base -- will always be in the message schema, or the Bind would not have worked.
"user" (or which ever field name was used in users.Bind(...)) gets set in then message item base if present in the message schema.

The last point probably isn't super-important, but would keep current schemas working. What do you think @Dragomir-Ivanov?

Dragomir-Ivanov · 2019-01-28T13:05:05Z

@smyrman Yes, I think I understand. Will make a PR later today. Thanks, and please take a look at #232 when have some free time. I think that it is a bite-sized one.

Fix for field validation on nested sub-resources

6530cdf

Dragomir-Ivanov mentioned this pull request Jan 29, 2019

Fix creating sub-resource items deeper than 2nd level #233

Merged

smyrman closed this in #233 Feb 7, 2019

Dragomir-Ivanov mentioned this pull request Sep 4, 2019

Question - Support for multi-level resources? #144

Closed

Fix for field validation on nested sub-resources #231

Fix for field validation on nested sub-resources #231

Uh oh!

Conversation

p7900 commented Jan 19, 2019

Uh oh!

Dragomir-Ivanov commented Jan 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

p7900 commented Jan 19, 2019

Uh oh!

Dragomir-Ivanov commented Jan 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

p7900 commented Jan 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Dragomir-Ivanov commented Jan 19, 2019

Uh oh!

p7900 commented Jan 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Dragomir-Ivanov commented Jan 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

p7900 commented Jan 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Dragomir-Ivanov commented Jan 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

p7900 commented Jan 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Dragomir-Ivanov commented Jan 19, 2019

Uh oh!

Dragomir-Ivanov commented Jan 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

p7900 commented Jan 20, 2019

Uh oh!

Dragomir-Ivanov commented Jan 20, 2019

Uh oh!

smyrman commented Jan 28, 2019

Uh oh!

smyrman commented Jan 28, 2019

Uh oh!

Dragomir-Ivanov commented Jan 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smyrman commented Jan 28, 2019

Uh oh!

Dragomir-Ivanov commented Jan 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Dragomir-Ivanov commented Jan 19, 2019 •

edited

Loading

Dragomir-Ivanov commented Jan 19, 2019 •

edited

Loading

p7900 commented Jan 19, 2019 •

edited

Loading

p7900 commented Jan 19, 2019 •

edited

Loading

Dragomir-Ivanov commented Jan 19, 2019 •

edited

Loading

p7900 commented Jan 19, 2019 •

edited

Loading

Dragomir-Ivanov commented Jan 19, 2019 •

edited

Loading

p7900 commented Jan 19, 2019 •

edited

Loading

Dragomir-Ivanov commented Jan 19, 2019 •

edited

Loading

Dragomir-Ivanov commented Jan 28, 2019 •

edited

Loading