Add support for a Scanner interface.

[flimzy]

This duplicates the Scanner interface implemented by the standard library's sql package, extending echo's standard Binder to support arbitrary data types, so long as they implement this interface.

This solves #763.

[vishr]

So this is specifically for form data?

[flimzy]

It should work for any input type that Binder supports: HTML forms, multi-part forms, JSON or XML POSTs, etc.

[vishr]

Do you mean JSON codec takes care of calling Scan()?

[flimzy]

Sorry, no. My mind is a bit of a muddle.

This PR would directly affect three types of inputs:

• URL parameters (i.e. method = GET as here.
• Standard HTML forms, of type "application/x-www-form-urlencoded", as here
• Multi-part forms, of type "multipart/form-data", also here

Where it could potentially affect JSON, etc, is that by implementing the Scanner interface, individual form fields (of the above mentioned types) could expect JSON, which could then be automatically unmarshaled (by virtue of the Scan() method calling json.Unmarshal()). I don't know how common of a request that might be, but literally any type of data unmarshaling/decoding could be done on a per-field/per-data-type basis this way.

I hope things are clearer now.

[flimzy]

At this point, I believe this PR to be functionally complete. Further discussion or requests for clarification are welcome, of course.

[coveralls]

Coverage increased (+0.2%) to 85.852% when pulling 382d82d on flimzy:scanner into f10daac on labstack:master.

[coveralls]

Coverage increased (+0.2%) to 85.905% when pulling 6161aa0 on flimzy:scanner into f10daac on labstack:master.

[coveralls]

Coverage increased (+0.3%) to 85.997% when pulling 2277b66 on flimzy:scanner into f10daac on labstack:master.

[coveralls]

Coverage increased (+0.2%) to 85.914% when pulling 43e3d8c on flimzy:scanner into f10daac on labstack:master.

[coveralls]

Coverage increased (+0.2%) to 85.914% when pulling 5086812 on flimzy:scanner into f10daac on labstack:master.

[flimzy]

I spent some time thinking about this PR, and a couple issues came up in my mind.

1. This PR works with either a struct, or a pointer to a struct in the bound struct. No other pointer types are supported, so I'm not sure if supporting pointers-to-structs is the best way forward, or if a separate PR would be appropriate to add support for the base types as well. This is more a question of echo's philosophy, I suppose, than what is strictly appropriate for this PR.

2. The reason I chose to implement sql's Scanner interface was for compatibility with data types/structs that already implement this interface. But upon further reflection, I wonder if this is a good idea. This would make it impossible (or at least very difficult) to implement an sql Scanner that is different from the way forms are interpreted. Perhaps a better solution is to use a different method/interface name here in Echo. That does mean that datatypes which implement sql's Scanner would also have to implement the new interface, but doing so can be as trivial as calling the existing Scan() method, if it is appropriate.

   Again, this is a question of philosophy rather than of the technically correct answer. I'm not sure which is best. If a separate interface is chosen, perhaps something along the lines of FormScanner or 'UnmarshalForm` would be appropriate, borrowing idioms from sql's package, and from the json package.

[JaTochNietDan]

I think this is a good solution to the problem. It will allow anyone to specify their own unmarshalling logic for their own types by implementing the Scan() method on the type. Right now if someone wanted to do something outside of the norm then they'd have to implement their own custom binder, which I think is totally un-necessary. It's a lot more work to maintain than simply maintaining a Scan function for the types they want to support, not to mention they may have already done that because they need it for SQL scanning anyway.

[coveralls]

Coverage increased (+0.3%) to 85.952% when pulling a7928cf on flimzy:scanner into f10daac on labstack:master.

[flimzy]

After discussion with my own team, I decided that borrowing the Scanner interface isn't ideal, so have renamed the interface in this PR. Alternate naming suggestions are welcome.

If this is accepted for merging, I should rebase to squash these commits.

[coveralls]

Coverage increased (+0.3%) to 85.944% when pulling 2e7d4eb on flimzy:scanner into f10daac on labstack:master.

[coveralls]

Coverage decreased (-3.9%) to 81.462% when pulling ba9ac64 on flimzy:scanner into 01334bc on labstack:master.

[coveralls]

Coverage increased (+0.3%) to 85.654% when pulling 0c206bd on flimzy:scanner into 01334bc on labstack:master.

[vishr]

Bind data is not limited to forms, it can also bind query parameters, so naming it with form won't be appropriate. How about:

type interface Unmarshaller {
  Unmarshal(data string) error
}

data could be []byte?

[flimzy]

@vishr Both great suggestions. I wonder, though, if Unmarshal might be too generic? How would you feel about EchoUnmarshal, or something along those lines?

[JaTochNietDan]

I would say Unmarshal with []byte is the way to go as it's then the same as the JSON and XML decoders.

[flimzy]

I would say Unmarshal with []byte is the way to go as it's then the same as the JSON and XML decoders.

It's actually UnmarshalJSON and UnmarshalXML, which is precisely why I suggested something more specific. UnmarshalEcho might not be ideal, though, since Echo isn't a data format. It's also why I chose UnmarshalForm previously, but @vishr is quite right that Form is too specific.

[vishr]

How about this?

type Unmarshaler interface {
        UnmarshalData([]byte) error
}

[JaTochNietDan]

Looks good to me.

…

On Wed, 14 Dec 2016 at 17:08 Vishal Rana ***@***.***> wrote: How about this? type Unmarshaler interface { UnmarshalData([]byte) error } — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#764 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB8uSZ0_WpNWVkcnv2r1FEW2dqcIqYFIks5rIBSGgaJpZM4LFtfs> .

[vishr]

If you look into the code, we are binding string, so UnmarshalString(string) error could also be an option.

Update:

Another point in favour of Unmarshal is that it is suppose to bound to struct and decode itself and not any kind of codec. With that you are free to implement it the way you want, you may decode it with json or xml inside. Let me know your thought.

[flimzy]

You make a good argument in favor of Unmarshal, @vishr. My only concern with that name is that it might possibly collide with some other interface. But if everyone is as thoughtful as we are, that hopefully won't happen. :)

What about UnmarshalParam? "Param" is a name used to reference both query parameters, and form parameters, if I'm not mistaken.

In any case, we've probably done enough bike shedding. Whichever your preference is, @vishr, is fine with me, and I'm happy to make the changes for this PR.

[vishr]

UnmarshalParam(param string) error sounds good. Please extend the docs (guide/request) to mention this - few lines should be good.

PS: Isn't interface name Unmarshaler too common? How about ParamUnmarshaler or BindUnmarshaler?

[flimzy]

Interface names don't collide, since they, like other data types, are per-package, and can even be private (unexported) and still work just as well. So we can call it whatever we want. ParamUnmarshaler seems fine to me.

[vishr]

I understand, I was concerned within the package as it is too general. In Echo, we usually prepend with the behavior like Log, Bind as package is flat. Let's call it BindUnmarshaler.

[flimzy]

Oh of course... within the package, yes, you may want other unmarshalers as well.

[coveralls]

Coverage increased (+0.3%) to 85.654% when pulling 109e3fb on flimzy:scanner into fa280ce on labstack:master.

[flimzy]

I have added a mention in the documentation; let me know if it should be expanded, or further mentioned elsewhere.

I also squished the commits, as they were becoming somewhat long-winded and verbose.

[coveralls]

Coverage increased (+0.3%) to 85.654% when pulling defa53c on flimzy:scanner into 874d336 on labstack:master.

[flimzy]

At one point you suggested using UnmarshalParam([]byte) instead of UnmarshalParam(string), but then mentioned passing a string again. Do you want me to change that to []byte?

Internally echo uses string there, but in theory []byte can be more efficient, so if the internals ever change to using []byte, it might be preferred to do that. shrug I have no strong opinion on that detail myself. :)

[vishr]

Lets keep it string, as param will always be string in our case.

[vishr]

thanks for your contribution 👍