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

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

Merged
merged 1 commit into from
Aug 26, 2015

Conversation

MaxGabriel
Copy link
Member

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
Copy link
Member

gregwebs commented Jun 6, 2015

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
Copy link
Member

gregwebs commented Jun 6, 2015

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
Copy link
Member Author

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

@gregwebs
Copy link
Member

gregwebs commented Jun 7, 2015

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

@gregwebs
Copy link
Member

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
Copy link
Member Author

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

@MaxGabriel
Copy link
Member Author

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
Copy link
Member

Thanks for the change.

  • I always use an Id suffix such as commentUserId because I often want to make a joined form without the Id
  • insertEntity seems wrong

A test and CSRF protection would be great.

@MaxGabriel
Copy link
Member Author

@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
Copy link
Member

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


var message = $("##{rawJS commentTextareaId}").val();
// (Browsers that enforce the "required" attribute on the textarea won't see this alert)
if (!message) {
Copy link
Member

Choose a reason for hiding this comment

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

What browser doesn't enforce required? Can be removed since we always rely on the server for final validation?

Copy link
Member Author

Choose a reason for hiding this comment

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

@gregwebs Currently the server won't let invalid comments be created, but it also won't give a nice error message to show to the user (IIRC the default error for requireJsonBody actually returns an HTML page with the error).

My general view on this is that client-side validation is fine (and even preferable, because it's instant) as long as it's just for UX, and the server is enforcing the actual integrity of the data, though it would be nice to get good errors from the server when it's own validation fails.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, sorry, Safari doesn't enforce required. Edit: and apparently mobile safari as well http://caniuse.com/#search=required

@gregwebs
Copy link
Member

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

\ defaultHeaders[csrfHeaderName] = csrfToken;
\ $.ajaxSetup({
\ headers: defaultHeaders,
\ });
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this should maybe only add the header if the domain the AJAX request is being sent to is the current domain. Need to look into this.

@MaxGabriel MaxGabriel force-pushed the jsonExample branch 2 times, most recently from ff72266 to 789170e Compare August 26, 2015 06:00
@MaxGabriel
Copy link
Member Author

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
Copy link
Member

published

* Includes frontend code making an AJAX request to post comments
* Demonstrates JSON parsing/encoding
* Includes CSRF protection
@MaxGabriel
Copy link
Member Author

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

MaxGabriel added a commit that referenced this pull request Aug 26, 2015
Draft of using Yesod as an API server to a Javascript frontend
@MaxGabriel MaxGabriel merged commit 5fca106 into yesodweb:postgres Aug 26, 2015
@MaxGabriel
Copy link
Member Author

Thanks for reviewing this so many times @gregwebs !

@gregwebs
Copy link
Member

Is it adding the token from the cookie?

@MaxGabriel
Copy link
Member Author

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

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.

2 participants