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

Remove old sub-resource syntax #252

Open
tgirod opened this issue May 15, 2019 · 4 comments
Open

Remove old sub-resource syntax #252

tgirod opened this issue May 15, 2019 · 4 comments

Comments

@tgirod
Copy link

tgirod commented May 15, 2019

I want a resource with an array of a sub-resource, and everything properly checked for validity.

As a preamble, I found two ways of declaring a sub-resource and I fail to see the difference between them. First approach:

Fields: schema.Fields{
    "sub": {
        Schema: &subresource
    },
}

Second approach:

Fields: schema.Fields{
    "sub": {
        Validator: &schema.Object{
            Schema: &subresource,
        },
    },
}

With that in mind, I found that using the first approach works when the subresource is a regular field, but not when declared inside an array. OTOH the second approach works in both cases.

var inner = schema.Schema{
	Fields: schema.Fields{
		"foo": {
			Required:  true,
			Validator: &schema.Integer{},
		},
	},
}

// approach using Validator:Object
var legit = schema.Schema{
	Fields: schema.Fields{
		"id": schema.IDField,
		"single": {
			Validator: &schema.Object{
				Schema: &inner,
			},
		},
		"array": {
			Validator: &schema.Array{
				Values: schema.Field{
					Validator: &schema.Object{
						Schema: &inner,
					},
				},
			},
		},
	},
}

// approach using Field.Schema
var bug = schema.Schema{
	Fields: schema.Fields{
		"id": schema.IDField,
		"single": {
			Schema: &inner,
		},
		"array": {
			Validator: &schema.Array{
				Values: schema.Field{
					Schema: &inner,
				},
			},
		},
	},
}

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

	index.Bind("bug", bug, mem.NewHandler(), resource.DefaultConf)
	index.Bind("legit", legit, mem.NewHandler(), resource.DefaultConf)

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

	// Bind the API under /api/ path
	http.Handle("/api/", http.StripPrefix("/api/", api))

	// Serve it
	log.Print("Serving API on http://localhost:8080")
	if err := http.ListenAndServe(":8080", nil); err != nil {
		log.Fatal(err)
	}
}

Running that code and POSTing some data, here is what I see:

# using the validator Object

$ echo '{"single":{"foo":42}}' | http POST :8080/api/legit -p b
{
    "id": "bjdrmssfhc2qebh96omg", 
    "single": {
        "foo": 42
    }
}
# OK. POSTing bogus data properly fails

$ echo '{"array":[{"foo":42}]}' | http POST :8080/api/legit -p b
{
    "array": [
        {
            "foo": 42
        }
    ], 
    "id": "bjdrn6cfhc2qebh96on0"
}
# OK. POSTing bogus data properly fails

# using the Schema parameter instead

$ echo '{"single":{"foo":42}}' | http POST :8080/api/bug -p b
{
    "id": "bjdrndcfhc2qebh96ong", 
    "single": {
        "foo": 42
    }
}
# OK. POSTing bogus data properly fails

$ echo '{"array":[{"foo":42}]}' | http POST :8080/api/bug -p b
{
    "code": 422, 
    "issues": {
        "single": [
            {
                "foo": [
                    "required"
                ]
            }
        ]
    }, 
    "message": "Document contains error(s)"
}
# BUG?
@smyrman
Copy link
Collaborator

smyrman commented May 20, 2019

This is related to #77.

Long story short, the second approach (the newest syntax) is recommended, and should work for more cases. The first approach (the earliest implementation of sub-schemas) should be deprecated, but some code still exists that only considers the earlier syntax.

My plan for solving this and other issues, is to do a proto-type for a new schema package design. However my progress have been (and is going to be) extremely slow.

@smyrman
Copy link
Collaborator

smyrman commented May 20, 2019

Since the scope of #77 has grown, and my progress is expected to be very slow btw. It might be good to keep this issue open, if anyone wants to fix it against the current design.

I would recommend to remove the Schema property in the schema.Field type, and updates the documentation to recommend using the Object validator approach for sub-schemas. When doing this, it's a good opportunity to spot any code that relies on the removed field, and rewrite it to work with an Object validator (preferably through use of interfaces, so that AllOff/AnyOff/Dict etc. will also work).

I am happy to accept/review any PR with such changes, but am afraid I won't have time to write the code.

@tgirod
Copy link
Author

tgirod commented May 20, 2019

@smyrman thanks for the explanation. I might have a swing at that next month.

@smyrman smyrman changed the title Bug with sub-resource inside an array Remove old sub-resource syntax May 21, 2019
@smyrman
Copy link
Collaborator

smyrman commented May 21, 2019

Renamed to reflect the work that is to be done.

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

2 participants