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

Implemented registerFormInterface. #106

Merged
merged 9 commits into from Oct 23, 2012
Merged

Conversation

eskimor
Copy link

@eskimor eskimor commented Oct 7, 2012

At the moment only the version taking a single method tested, but at least
this one works like a charm.

Still not meant to be pulled. But very close.

Still missing:

  • Complete test
  • Improve documentation
  • Write unit tests where it makes sense.

Getting form data as a single struct parameter will be the target for another pull request.

@s-ludwig
Copy link
Member

s-ludwig commented Oct 8, 2012

Looks good. I've thought about your suggestion of putting it into form.d instead of rest.d and, although form.d will still get some additional functionality, I think it's a better choice (and I don't really like another forminterface.d module). The shared functionality can stay in rest.d for now (and marked as package) as there is another pull request still coming up.

Tell me when you want it merged. Later, when the library in general is considered stable, new features will also have to be high quality. But for now merely working towards a high quality implementation is also OK, even if it means multiple interations.


See_Also: registerFormMethod

Parameters:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be "Params" (it's a standard DDOC field name that is handled differently)

@s-ludwig
Copy link
Member

Apart from the minor formatting stuff (I also wouldn't mind to fix it) I'd say this looks pretty good. Do you have anything left that would hold this pull back?

@eskimor
Copy link
Author

eskimor commented Oct 19, 2012

Well it is still not completely tested yet, especially the code extracting methods from a class. I only tested registering a single overload. Apart from this, I wanted to review everything, proof read documentation and improve it+examples. Write unit tests. I am very sorry that I haven't done this already, but I am quite busy at the moment in preparation for an exam.

If this does not bother you, feel free to pull. It can't be too broken.

@eskimor
Copy link
Author

eskimor commented Oct 19, 2012

Thanks for having a look! :-)

@s-ludwig
Copy link
Member

No problem at all, time doesn't matter. I was just asking because apart from missing unit tests it looks quite good and I wasn't sure ;)

Missing unit tests is an open topic for many modules in vibe anyway - I wanted to do a pass through everything and add better documentation and unit tests, but up to now it was more important to have all the features ready (to actually get some stuff done with it).

@eskimor
Copy link
Author

eskimor commented Oct 19, 2012

sure ;-) I think I might find some spare time this weekend for making it ready and awesome.

At the moment only the version taking a single method tested, but at least
this one works like a charm.
Implemented example showing its use. By means of this example it is also
already basically tested. Although I want to run some more tests before
considering it for release. Also the example is not complete yet, the
method for posting data to the array is not yet used.
@eskimor
Copy link
Author

eskimor commented Oct 21, 2012

Ok. Everything seems to work as expected. I will finish up the example code I wrote and add some more tests. But if you want you can pull already.

Everything is working fine. The example pretty much serves as a unit
test for now, as it covers most if not all functionality.

Little doc fix in form.d
@eskimor
Copy link
Author

eskimor commented Oct 22, 2012

Ready, it works really well for me. The example now covers all functionality (at least I think so). I will leave unit tests out for now, but the provided example is a pretty good test (although not automated).

@s-ludwig
Copy link
Member

Great, I'll merge it in then. Also good that it will be in the next release, this should be a nice small productivity booster for the typical web front end app. (It overlaps a bit with zeal.d in that regard, which I unfortunately never had the time to test until now).

s-ludwig added a commit that referenced this pull request Oct 23, 2012
Implemented registerFormInterface.
@s-ludwig s-ludwig merged commit 1094416 into vibe-d:master Oct 23, 2012
@eskimor
Copy link
Author

eskimor commented Oct 23, 2012

It seems to also overlap with registerRestInferface? If it is actively maintained, maybe it can be changed to make use of the implementations in vibe, I would be glad to implement any improvements that might be needed.

@eskimor
Copy link
Author

eskimor commented Oct 23, 2012

Great :-)

@s-ludwig
Copy link
Member

Oh of course it overlaps just as with the REST version, didn't think in that direction ;)

Btw. we should think about the best way to handle method names/HTTP methods. Allowing both, GET and POST was a suggestion from the D community. But at least in my own projects I always keep them cleanly separated in the sense that GET requests do not mutate state - so I don't really think this is the best idea.

However, using a full REST style interface may be too much, especially since anything other than GET/POST may still be unreliable when used from a browser/through a proxy. But then again, when mapping e.g. DELETE to POST, it does not make sense to strip the "delete"/"erase" prefix off the method name.

So my take on this would basically be to use adjustMethodStyle as is, but remove the if clauses for anything non-GET/POST so that it maps to the default POST case. Do you have any other ideas or preferences for this?

@eskimor
Copy link
Author

eskimor commented Oct 23, 2012

Well using a full REST style interface might be a good idea after all.

As far as I get it from reading the code of registerRestInterface friends, I see that you fixated the API pretty much on JSON. Is there a particular reason for this? If not, the registerFormInterface might not be too different from registerRestInterface and it might make sense to make the method handler configurable, in this case my code could be easily merged in for handling HTML style communication.

In the example I provided with the DataProvider class I am using the registerFormInterface for generating a pretty much REST like interface, the method names would need to be adopted, but that's pretty much it. In the way I use it with the App class is just for convenience and not very REST like, but I don't consider this a problem. Encouraging developers to REST like programming is not too bad I think and providing an easy way to expose a REST interface based on HTML, might not be a bad idea at all. In addition, I could adopt the formHandler so it renders the return value of the supplied function if it does not take the req, res parameters.

I am pretty new to Web development in general, what's the problem with proxys and put,delete, ..? Ah I see, it seems that this is HTTP1.1 specific? Isn't this then a problem in general?

On the GET/POST issue I agree with you, I just left it because you had it this way and it seemed to me to be common practice.

What's your thinking about making the registerRestInterface more flexible? Aliases or templates for the specialized stuff could easily be provided for convenience. Any problems, objections? I would volunteer for the job.

It might also be a good to pass the HttpServerResponse object on to the REST inferface method if it defines the appropriate parameter, so it can set things like caching?

@s-ludwig
Copy link
Member

The RestInterface stuff is supposed to provide a standard JSON protocol on one side and a normal D style interface on the other side (a kind of RPC proxy). The D programmer shouldn't have to worry about HTTP, JSON query strings etc. This is why I'm a bit reluctant about extending it to be able to take the HTTP request. It's a pity that D does not support user @attributes - this would allow things like cache control in a very elegant way.

The form interface on the other hand would be more for designing the HTTP interface of a service with more control and more focus on the HTTP side and less focus on the D side interface.

I also thought about providing a default render of return values and this probably is a good idea. The only question is which format. JSON is probably a good all-round choice...

The problem with the more unusual methods is that older HTTP proxies often drop the corresponding requests because they detect them as invalid. And no finished HTML standard currently supports form requests with anything else apart from GET/POST (some browsers do support them anyway). So they still have to be used with care. But thinking about what I just said (less focus in the D side interface) - it's probably not too much to ask to name a method postDeleteItem instead of deleteItem... So maybe using the current adjustMethodStyle with the exact methods would be OK after all.

@eskimor
Copy link
Author

eskimor commented Oct 25, 2012

I think it is not too bad to use postDeleteItem for compatibility reasons, but I will continue to think about it. At the moment I think it is good to keep semantics the same as for registerRestInterface. Especially I don't really understand the arguments that caused put/delete not to be in the standard. The argument I read was that delete usually delivers just an 'Ok' which is pretty useless for a browser, but in my understanding I am in full control of what put/delete returns and if I know that it is triggered from a form, I might as well return a full web page. If this thinking has no serious flaw, I think it is best to support put/delete in vibe, if browsers don't support it, simply use a script or fallback to your suggested 'postDeleteItem'.

My thinking was just, that apart from the handler the code for registerRestInterface and registerFormInterface can be made pretty much the same very easily. The idea is simply to provide the rest interface stuff upon a more generic solution.

If people also want to use the RestInterfaceClient, which btw. is pretty awesome, then they would simply have to use registerRestInterface just as now, it should not really matter that registerFormInterface uses the same code. So it is not about extending the registerRestInterface to support different formats than JSON, I think when dealing with scripts this is almost always the best format, but base it on a registerFooBar() that takes the handler as a parameter. registerFooBar() would then be used also by registerFormInterface. ( FooBar to be replaced with something meaningful, of course) The ":id" stuff might also make sense for registerFormInterface, I am not so sure for the return-interface part. So maybe forget about it for now, future will tell if it makes sense at all.

I don't think that adding HTML style output to registerRestInterface would be a good idea. In my understanding, registerFormInterface and registerRestInterface operate quite on different layers. registerRestInterface is for backend functionality which is made available to scripts and other web applications, but not UI related. While registerFormInterface is part of the UI layer, which not only provides the data, but also their representation. Its main use case would simply be convenience, because you avoid the necessity of handling the form data directly and also as demonstrated in my example to enable AJAX style with fallback to traditional "load whole page" with maximum code reuse.

As direct consequence of this view, I re-thought handling of return values in registerFormInterface: As registerFormInterface purpose is mainly for easy handling of forms, I think it would be easiest to just ignore the return value. A general return value serialization seems to be only reasonable for scripts, which usually should be better off with a registerRestInterface interface. If we decide to serialize the return value, JSON might be the best option for compatibility with registerRestInterface. (at least for get requests, scripts would not note the difference)

For handling overloads, If I understand correctly from the code, you are not handling multiple overloads of the same function? If so, is this on purpose or would you appreciate it, if I improved it in this regard?

On a different matter: I read a bit about REST style interfaces and what is mentioned that the API should be self explorable without prior knowledge or out-of-band communication, which I think is a quite good approach. My idea to accomplish this goal would be to automatically provide a JSON content for '/' if no index method is found, which lists in JSON format all available methods. What do you think? The urls referring to methods returning an interface would then also show their contents.

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

Successfully merging this pull request may close these issues.

None yet

2 participants