Skip to content
This repository has been archived by the owner on Jan 22, 2019. It is now read-only.

Allow nested structured in payload to be considered by "require_params" #23

Open
sukria opened this issue May 29, 2013 · 8 comments
Open
Assignees

Comments

@sukria
Copy link

sukria commented May 29, 2013

Currently, SPORE only checks for keys in the params structure that it's passed. This is fine with GET request, or QUERY_STRING args but not when it comes to provide a value in the structure given in payload.

For instance, let's say I have

required_params: 
  - "foo"
  - "bar"

If I do the following:

$spore->some_action( payload => { 
  foo => 42,
  bar => 43,
});

This will fail, triggering a "required param is missing" error, which is wrong, as the user submitted the expected parameter, in the body.

Probably, we should also allow nested checks like :

required_params:
  - "object"
  - "object.key1"

$spore->some_action( payload => { 
  object => {
     key1 => ... 
   }
});
@ashb
Copy link
Contributor

ashb commented May 29, 2013

Something like this is probably a good idea but involves a change to the SPORE spec - tag @franckcuny

The one point of contention is that various languages/frameworks serialize nested structures in different ways (object.key vs object[key] for instance. Not sure off the top of my head what does which.)

@ghost ghost assigned fcuny May 29, 2013
@fcuny
Copy link
Collaborator

fcuny commented May 29, 2013

To be honest I've tried to stay away from this. If you want to do it right, I don't think you can only do it for the payload, but you will need to do it for the response. So you need a way to define the request/response objects, and then you'll need to define the type, to be sure you're sending the correct content, and do validations for the types, which can be complicated for a lot of languages.

That's what Google does for the Discovery API, and it's really complex and not easy to port to all the languages (e.g.: the way languages serialize nested structures, like mentioned by @ashb). We also have something very similar at work, where we need to generate the code from the type's description, and it's not really easy to use :/

I can try to work on a proposal, and I'll like @ngrunwald and @fperrad opinion on this.

@sukria
Copy link
Author

sukria commented May 30, 2013

Hello @franckcuny and thanks for the feedback. I understand what you and @ashb explain, but take a little example (from our real-life case):

We have a POST /create_object action which takes - in the body, encoded as JSON, an "object" hash, for instance:

$spore->create_object(
payload => {
  object => {
     name => "Foo",
     owner_id => 42,
     ...
  }
...

In our case, the entry object is mandatory, and inside, name and owner_id as well. In the current implementation/spec of SPORE, this brings the following configuration:

  ...
  methods:
    ...
    create_object:
      method: POST
      path: /create_object
    ...

We just can't say more. The benefit of SPORE is highly reduced compared to simple GET actions, and that is what we would really like to fix there.

I understand it's more complicated than just fixing the Perl implementation, we need to keep in mind the Spec side, and other languages, but what about the following:

We could use a dot notation in the spec, like "object.label" to declare that an entry object is to be found, which should contain a nested structure that has an entry "label".

I don't see how this is impossible to implement in other languages, am I missing something?

BTW: I agree this discussion could be a better fit in the SPORE Spec project.

@sukria
Copy link
Author

sukria commented May 30, 2013

Also @ashb we could do the check before the serialization occur, right? Or am I missing something?

@ashb
Copy link
Contributor

ashb commented May 30, 2013

@sukria Sorry, which check?

It's definitely not impossible to implement in other languages.

I think the easiest plan forward might be to allow the validation to be easily pluggable then if we can't agree on a way to get this into core then it would at least be easy for you to add this (either in your app or in another CPAN dist).

Though I'd like to see if we can agree on a 'core' way of doing this first if we can.

@sukria
Copy link
Author

sukria commented May 30, 2013

👍 indeed, it really belongs to the core, IMHO.

@fcuny
Copy link
Collaborator

fcuny commented May 30, 2013

So, what I was trying to describe is this. In your example you have the following payload:

payload => {
  object => {
    name => "Foo",
    owner_id => 42,
    ...
  }

I believe that the right way to deal with that is to say that your payload is expecting an object like this:

payload => 'object<typeofobject>'

where typeofobject is described like this:

typeofobject:
name: str
ownder_id: int

The client then see that payload is a string with the specific markup object. This way you can get validation on the type of payload (it's an object of type typeofobject with all the expecting fields, and they have the correct type).

It's more complex and not what I want with SPORE (I don't want to have to deal with objects and types, because that could be difficult to implement for some languages).

I don't really like the idea of using a '.' as an indicator that we're expecting a nested object, what will happen if you have a key with a dot ?

@sukria
Copy link
Author

sukria commented May 31, 2013

I see your point @franckcuny and I agree it can quickly become tricky. But I'm a bit puzzled anyway, because in all non-GET calls, that means that we can't have simple validation for the payload content. It's quite odd.

I was looking for a good-enough solution that would be better than the current state, but if that is not wanted here, I'll do it in a separate module, I guess.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants