yesodweb / yesod-scaffold

The Yesod scaffolding, with branches for different versions.
MIT License
76 stars 39 forks source link

Draft of using Yesod as an API server to a Javascript frontend #76

Closed MaxGabriel closed 9 years ago

MaxGabriel commented 9 years ago

In #73 there was some discussion about adding an example of using Yesod as an API server. I took a quick stab at that; this PR just adds a link you can click on the homepage, which triggers an AJAX request to the server to create a new user. The server inserts the user into the database and returns the data to the client, which updates adds the user to an <ul>.

Is this roughly what we're looking for? I wrote functions to request all users and to delete specific ones as well, if we want to add more API server example code.

gregwebs commented 9 years ago

Thanks for taking a stab at it! Unfortunately this specific example is very problematic from a security perspective. If instead of a User it used a different (new) model (lets say SiteComment), then it would be great.

gregwebs commented 9 years ago

Interestingly someone just made a similar example for servant. https://github.com/parsonsmatt/servant-persistent/blob/master/src/Main.hs Note that for servant they don't bother presenting a UI in the browser. However, for us not using the browser means adding API authentication, but Yesod is setup out of the box for cookie authentication.

I still maintain that an an API to users is normally admin code. Adding admin code to the scaffolding would be quite useful, but it seems out of the scope of this PR.

MaxGabriel commented 9 years ago

@gregwebs Good point about the security issues. Updated based on your suggestion to create comments instead.

gregwebs commented 9 years ago

Want to add an optional UserId field to the Comment to make it more realistic?

gregwebs commented 9 years ago

sorry, I didn't notice the update. The UserId should come from maybeAuthId. Sorry for asking for tedious changes; I can also finish this off later.

MaxGabriel commented 9 years ago

It's no problem! Updated to use maybeAuthId and insertEntity.

MaxGabriel commented 9 years ago

I could also add comments to all of this code, much like Application.hs has. As well as tests. I also realized that postCommentR doesn't have CSRF protection. Should I add those things?

gregwebs commented 9 years ago

Thanks for the change.

A test and CSRF protection would be great.

MaxGabriel commented 9 years ago

@gregwebs I agree on adding an Id suffix to the record field; I only didn't do so because it'd be inconsistent with Email:

Email
    email Text
    user UserId Maybe
    verkey Text Maybe
    UniqueEmail email
Comment json
    message Text
    user UserId Maybe

I can change both, though.


For insertEntity, that's just this code, which is doing a regular insert and using the returned key to create the Entity. The only thing I see potentially wrong with that is that if the database makes changes to your data during the insert (with triggers or default values or something)—but that seems like a general problem with insertEntity.

gregwebs commented 9 years ago

sorry, I got confused about insertEntity. I would like to change both to have the Id suffix.

gregwebs commented 9 years ago

This looks like it will be good to merge when rebased.

MaxGabriel commented 9 years ago

This PR needs yesod-test-1.5.0.1 for the tests to pass at runtime. Do you mind publishing that @gregwebs or @snoyberg? After that's merged I'll bump bounds on this branch, but otherwise this PR is good to merge.

gregwebs commented 9 years ago

published

MaxGabriel commented 9 years ago

Updated cabal file to require yesod-test-1.5.0.1. Since Greg's reviewed this I'll merge shortly.

MaxGabriel commented 9 years ago

Thanks for reviewing this so many times @gregwebs !

gregwebs commented 9 years ago

Is it adding the token from the cookie?

MaxGabriel commented 9 years ago

@gregwebs Yes, in default-layout-wrapper.hamlet