martini-contrib / binding

Martini handler for mapping and validating a raw request into a structure.
MIT License
140 stars 47 forks source link

Map a pointer of a struct to an interface #24

Closed yosisa closed 10 years ago

yosisa commented 10 years ago

I want to map a pointer of a struct to a specific interface, but I can't. I read https://github.com/codegangsta/martini-contrib/pull/34#issuecomment-29683659 and I see the current limitation is introduced to avoid race conditions.

But I think this limitation is not necessary, for example, after changing line 402 of binding.go

context.MapTo(obj.Elem().Interface(), ifacePtr[0])

to

context.MapTo(obj.Interface(), ifacePtr[0])

it still works (although it requires a struct to implement the interface methods with a pointer receiver). Because that obj is created by reflect.New each time a request comes in, there are no race conditions.

mholt commented 10 years ago

Hmm, you may be right... but I haven't had a chance to experiment. I'd be interested to see what is involved to allow that to happen such that all the tests still pass (obviously, the test for ensureNotPointer may not be necessary any more, so other than that).

yosisa commented 10 years ago

I have an idea.

  1. A first argument of Handlers might be a pointer of a struct.
  2. Change ensureNotPointer to return two values: a new struct created by reflection and a flag which shows whether a value struct passed as a pointer or not.
  3. Inside validateAndMap, map the new struct (to an interface also) as a pointer if the flag shows the value struct originally was a pointer. Otherwise, nothing is changed.

What do you think about this way?

mholt commented 10 years ago

I am glad that you did some digging enough to discover that the race condition may not actually exist -- I haven't had a chance to confirm this, but what you're saying makes sense. However, it seems like this process would add some complexity to the package, which is something that we'd like to avoid. What are the benefits of passing in a pointer?

attilaolah commented 10 years ago

As far as I can see, the benefit of passing in a pointer is that the mapping is done on the pointer, so you could "ask for" a pointer in the handler. That way one could save some memory with large objects by not making a copy.

For small objects I think just using a struct is fine. You could still implement Validate() with a pointer receiver and the copy you get would be a modified copy, if you modify it in Validate().

yosisa commented 10 years ago

At first, I think passing a pointer is useful because a pointer receiver can modify its values, for example, generating id if not exists or filling current time to updated_at. But now, as @attilaolah said, I found that it is possible using Validate() method and my motivation has gone.

In my opinion, implementing interface methods with a pointer receiver is common case, so I have still believed that this is useful and less confusing. But I'm not sure it's good for everyone.

mholt commented 10 years ago

Sounds like a good solution to me, more semantic and simpler too.

yosisa commented 10 years ago

Sorry I had missed an important thing: Validator.Validate() is not invoked with DI of martini, thus I can't access needed values like martini.Params, a database interface or something else. I'm writing a REST API server and I want to overwrite the id field by a portion of a url using martini.Params. @mholt do you have any plan to use DI for validation?

attilaolah commented 10 years ago

That sounds cool, but then you would have to do that with a blank interface, since the signature of Validate() is no longer defined. So Validator would have to be an interface{}, or perhaps a martini.Handler, which is basically the same thing.

Still, it would be cool to have DI there :)

mholt commented 10 years ago

Hmm. I think if Validation was made into separate middleware, it should be a different martini-contrib repository. (I'm not opposed to the idea, but I probably won't head it up myself.)