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

feat: add single message path #48

Merged
merged 27 commits into from Mar 1, 2018
Merged

feat: add single message path #48

merged 27 commits into from Mar 1, 2018

Conversation

apetro
Copy link
Member

@apetro apetro commented Feb 23, 2018

Adds paths for viewing just one message, identified by id.

  • /admin/message/{id} will view the message regardless of audience
  • /message/{id} will view the message iff the message is available in the context of the request (so, considering who is asking, current date vs begin and end dates, etc.)

Why? To bootstrap idiomatic error handling in this Spring Boot microservice, namely

  • trying to view a non-existent id should yield 404 NOT FOUND
  • non-administratively trying to view a message you're not in the audience for should yield 403 FORBIDDEN

What this demonstrates:

  • Controller returning null automagically is 404 NOT FOUND. (Not true! Moral of story is always ensure your unit test fails before it succeeds, otherwise maybe you forgot to @Test annotate, as I did.)
  • Controller throwing automagically is 500 unless the exception is annotated (as the custom exceptions here are), then it's whatever the annotation said (here, 403 FORBIDDEN or 404 NOT FOUND).
  • Domain exceptions can be generated down in the Service layer and beyond where they're really happening, let them percolate up out through the Controller, it's all good.
@ResponseStatus(value = HttpStatus.FORBIDDEN, 
  reason = "Requesting user not in audience for requested message")
public class UserNotInMessageAudienceException extends RuntimeException {

Tradeoffs navigated

Explored not using Exceptions to model errors out of the Controller layer. That wasn't much fun. SpringWebMVC really "wants" to use annotated Exceptions for this.


Contributor License Agreement adherence:


Definition of Done

  • Documented
  • Fails gracefully
  • Tested
  • Secure
  • Adds no defects
  • Shippable. Path is clear to promote to production
  • Maintainable
  • Configurable
  • Readable
  • Validates and sanitizes user input
  • Styled (Google Style)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 61.25% when pulling 857268f on service-one-message into 9feae9e on master.

@coveralls
Copy link

coveralls commented Feb 23, 2018

Coverage Status

Coverage increased (+11.3%) to 72.187% when pulling 195c7ea on service-one-message into 4370efa on master.

@apetro apetro changed the title WIP: feat: add single message path feat: add single message path Feb 26, 2018

public ExpiredMessageException(Message messageWithRequestedId, LocalDateTime asOfWhen) {
super("Message with id [" + messageWithRequestedId.getId() + "] is expired as of " + asOfWhen
.toString());

Choose a reason for hiding this comment

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

This exception will throw an exception if asOfWhen is null

"description": "This announcement is not live. Too soon..",
"descriptionShort": "Premature.",
"messageType": "announcement",
"goLiveDate": "2999-01-01",
Copy link
Contributor

Choose a reason for hiding this comment

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

This will fail with the new refactored filter structure. Let's merge #49 first, and then fix this with the new structure.

@apetro apetro changed the title feat: add single message path WIP: feat: add single message path Feb 26, 2018
@apetro
Copy link
Member Author

apetro commented Feb 26, 2018

Intend to, in a new branch off of master, rebase these changes off of latest master to yield a clean PR. Will close this PR in favor of that when it's available. Will reset this PR's branch to that new branch, effectively "rebasing", when that is ready, thereby keeping the history of this conceptual branch in this single PR.

Pushed the new clean-one-msg branch, and cut over to it.

@davidmsibley
Copy link

Could the new custom exceptions be checked Exceptions rather than RuntimeExceptions? Runtime exceptions lead to not so funtime surprises.

@apetro
Copy link
Member Author

apetro commented Feb 28, 2018

I'm willing to give on this, but I thought RuntimeExceptions rather than checked exceptions were idiomatic in the context of the Spring framework. As I recall Rod Johnson's book waxed poetic on this point.

(Discussed f2f. Team consensus is regardless of whether RuntimeExceptions might be in the spirit of Spring Framework idioms, here use checked exceptions.)

(This is one of those moments where I'm entirely convinced I'm right to use RuntimeException, yet the marginal value in being correct is negligible (as it sometimes is!), so quite happy to bend to team consensus.)

@apetro apetro changed the title WIP: feat: add single message path feat: add single message path Mar 1, 2018
@apetro apetro merged commit 2eb9343 into master Mar 1, 2018
@apetro apetro deleted the service-one-message branch March 1, 2018 17:19
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

6 participants