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

🗄🔧 Kotlin/Spring #93

Closed
3 of 5 tasks
agrison opened this issue May 4, 2017 · 24 comments
Closed
3 of 5 tasks

🗄🔧 Kotlin/Spring #93

agrison opened this issue May 4, 2017 · 24 comments

Comments

@agrison
Copy link
Contributor

agrison commented May 4, 2017

Current Status

Repository (WIP):

agrison/realworld-kotlin-spring

Help

Come and help improve the codebase 😃

Todo:

  • 🏁 Fork the starter repo & post the link in this issue
  • 🎨 Create logo for repo & update issue status (@EricSimons)
  • 🔨 Implement all of Conduit's functionality per the spec & API
  • 👀 Move repo to main org & Peer review final codebase by admins/community (RFC)
  • 🎉 Tag v1 release and officially list it on the README!

Specifics

Spring Data REST

I did chose not to use Spring Data REST because it gets complicated to remove the _embedded element as it's a fully HAL+JSON compliant library which does not work well with the API spec of the real world app project.

Spring Security

Regarding Spring Security, I think its too complex to implement for such a simple use case (extracting the Authorization header, check in db then parse the JWT).

Database

For the moment it uses a simple H2 in memory db, but it can be changed easily.

Testing

Testing needs to be implemented

@agrison agrison changed the title Kotlin/Spring 🗄🔧 Kotlin/Spring May 4, 2017
@agrison
Copy link
Contributor Author

agrison commented May 4, 2017

Tested it with the react frontend, seems OK.

What's the process now @SandeeshS ? Should I host a demo somewhere?

@sandeesh
Copy link
Member

sandeesh commented May 4, 2017

@agrison oh wow you've already completed it? 👍 Generally you fork the repo and complete the codebase with regards to the spec. Once complete you do a thorough test and give us the go from your end, after which we test the functionality once and tag it for rfc for a peer review. After all tests pass we can move the repo here :)

@agrison
Copy link
Contributor Author

agrison commented May 4, 2017

@SandeeshS Which front is the reference for testing, so that I'm sure that my tests are correct before I give you the go?

@sandeesh
Copy link
Member

sandeesh commented May 4, 2017

@agrison you can use any of the three referenced on the main page. React or angular is what most use to test.

@agrison
Copy link
Contributor Author

agrison commented May 4, 2017

OK I'll ping you once I tested it all :)

@agrison
Copy link
Contributor Author

agrison commented May 4, 2017

@SandeeshS I tested all functionalities, everything's OK from my POV 👌
I used the react frontend.

@EricSimons EricSimons added rfc and removed wip labels May 5, 2017
@sandeesh
Copy link
Member

sandeesh commented May 5, 2017

@agrison awesome 👍 I've never worked with spring before. I'll get your project setup and give the api a test from my end and give you my feedback by today.

@sandeesh
Copy link
Member

sandeesh commented May 5, 2017

@agrison I got this setup locally and tested. I gotta say, the maven dependencies take a long time to download on windows machine. Most of the stuff seems to be working perfect except for these few things.

  • Wrong error response on failed login
  • No username and email validation check on registration and update
  • Deleting comments throw a 404 error, but actually get deleted. Only on refresh can we see them gone.
  • Comments could be sorted by latest first.

Note: Username needs a alphanumeric check and email with valid email type check.

I'll try doing another round of thorough test once you've made the changes and give the 👍 from my side.

@agrison
Copy link
Contributor Author

agrison commented May 5, 2017

Hi thank you for the testing. Maybe we should update the API spec because I would have known that it was needed to validate the username (alphanumeric) and email without bothering you.
I added these checks 😄

Regarding the wrong error response on failed login, it's not clear in the spec if a 401 or a 403 is needed. I was using a 401 so I conclude that I should respond with a 403 (which I did).

Comments are now sorted correctly.

Regarding the 404 on comment deleting, I can see through postman that it correctly sends a 200 OK with no body.

The changes have been pushed.
Oh, and for maven yes the first install can take a long time, since I'm using the snapshots of spring boot they can be modified from time to time and downloaded again, but the rest should not. Once it's done, the startup should be fairly quick (like 10 seconds on my mac).

Thanks 😃

@sandeesh
Copy link
Member

sandeesh commented May 6, 2017

@agrison Yes i understand. I've been meaning to get the api spec re done with the validation criteria and more. But haven't been able to find the time. You're also true about the failed login. I actually did make it return a 401 on fail on laravel backend, then i had to change it to 422 with failed login message for the current frontends to make it work properly. That's how the other 3 main backends have it implemented. I had to change a lot to match with those to keep things in sync. These things are indeed important and need to be documented better.

I actually did get a 404 response on comment deleting, but they were getting deleted nonetheless. I'll pull the new changes and have another test later tonight and give you my feedback.

@agrison
Copy link
Contributor Author

agrison commented May 6, 2017

OK.

So wait I'll fix the login to make it respond a 422 instead of a 401/403 ;-)

@agrison
Copy link
Contributor Author

agrison commented May 6, 2017

Done 😉

@sandeesh
Copy link
Member

sandeesh commented May 9, 2017

@agrison Sorry for the late response, been pretty busy lately. I pulled your changes and ran another test. Here are somethings you might want to check.

  • Deleting article or comment returns an invalid json response which the browser fails to handle.
  • No validation check on updating user. Checks on registering works fine.
  • Updating user info causes the token to become invalidated, thus causing further actions to throw unauthorized header error.

ss 2017-05-09 at 08 15 40

@agrison
Copy link
Contributor Author

agrison commented May 9, 2017

Hi @SandeeshS,

Deleting a comment returns nothing in the response body and sets the ResponseCode to 200 OK :
capture d ecran 2017-05-09 a 17 31 45

So does deleting an article :
capture d ecran 2017-05-09 a 17 32 14

You're right regarding the user validation while updating, it has been implemented yesterday but I didn't push the corrections :)

Updating the user information causes the token to be re-generated only if the email has changed (since the email is stored in the JWT). What is the correct implementation for you ? For me since the token is returned from the backend, if it has changed it should be stored on the frontend localstorage and used for next requests.

I'll open an issue because testing the backend was a bit tedious with PostMan so I created a small program that test a backend implementation regarding what's said in the API spec. Maybe it can help other people, or even you 😉

@agrison
Copy link
Contributor Author

agrison commented May 9, 2017

This is the tool I was talking about: https://github.com/agrison/realworld-server-tester

@sandeesh
Copy link
Member

sandeesh commented May 9, 2017

@agrison That sounds swell 👍

Well currently the token only gets stored in sign in, therefore token changes later wouldn't reflect. The ideal situation would be to use single request token as per the JWT standards. But this project doesn't require that high level of security since it's more like a starter, or that's what the idea was when the spec was first created. So i'd suggest not to invalidate the token at least to keep in sync with the other implementations. Btw none of the JWT claims are actually used by the frontend, it only cares and uses the token as a whole. So i'd even recommend not storing anything in the JWT apart from the standard claims.

As for the delete response errors, can you test with a local frontend setup? I have attached the error that i received on chrome using the angular frontend.

@agrison
Copy link
Contributor Author

agrison commented May 9, 2017

Ok for the JWT sounds ok for me.

I think the problem for article delete + comment delete are finished.

Delete comment:
capture d ecran 2017-05-09 a 18 00 34
capture d ecran 2017-05-09 a 18 00 52

Delete article:
capture d ecran 2017-05-09 a 18 01 32
capture d ecran 2017-05-09 a 18 01 14

No errors in the console at all.

@sandeesh
Copy link
Member

sandeesh commented May 9, 2017

@agrison cool, let me know when i can pull the latest changes and test it out one last time :)

@agrison
Copy link
Contributor Author

agrison commented May 9, 2017

Well you can 😄

@sandeesh
Copy link
Member

sandeesh commented May 9, 2017

Ran another test. All the other stuff is fixed and working as intended. The error on deleting article and comment seems to be something with the angular frontend. It's failing to parse the empty json response. Tried with chrome and ff and it's the same. You may look into it if you have the time.

All the tests have passed from my side 👍

@agrison
Copy link
Contributor Author

agrison commented May 9, 2017

Well its not specified in the API spec if deleting an article should return something or not. I made it return nothing, but it's easy to return anything else. For example the deleted article 😉
Same story for comment deletion 👍

@EricSimons
Copy link
Member

EricSimons commented May 10, 2017

@agrison yeah the API spec is currently a bit flawed (100% entirely my + @apai4's fault, putting this project together was a huge task and we unintentionally cut some corners). @SandeeshS has been working on a new version though that is really thorough & detailed, which should fix problems like these 💯

Regarding the codebase, things are looking good on my end as well. It would be great to have a Kotlin/Spring community member review the codebase to ensure it follows best practices/etc — do you know any off hand? No worries if not; I can try and chase some folks down :)

@agrison
Copy link
Contributor Author

agrison commented May 10, 2017

Like I said I only played with Kotlin for some time, I'm more versed into Java, Clojure or other languages. It was mainly for a fun experiment 😃
If you know people who can to take a look and/or participate that would be great to learn. As always there's always some code to remove, improve or change to make it more idiomatic 😉

@EricSimons
Copy link
Member

Awesome work!! Just created the official repo (https://github.com/gothinkster/kotlin-spring-realworld-example-app) and added @agrison as a collaborator/admin! Also added it to the main README :)

Closing this issue; any future discussions should happen over at https://github.com/gothinkster/kotlin-spring-realworld-example-app/issues 👍

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

No branches or pull requests

3 participants