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

Request Validation #14

Merged
merged 17 commits into from
Feb 11, 2022
Merged

Request Validation #14

merged 17 commits into from
Feb 11, 2022

Conversation

aneeshsharma
Copy link
Contributor

@aneeshsharma aneeshsharma commented Dec 30, 2021

As discussed earlier in #12, divided the models among API and DB models

  • Added API models in internal/models/api/
  • Moved DB models to internal/models/db/

Implemented some basic validation for a Create Queue request

  • Implemented Validate method on CreateQueueRequest
  • Right now it just checks the length of QueueName to make sure its between 4 and 20
  • Return a 400 HTTP error when the length is outside this range
  • Also return 400 HTTP error when unable to parse the JSON

Next steps would be to

  • Move Parsing of JSON to middleware
  • Move Validation to a middleware

I need to look into how to proceed with implementing middleware in such a way as to have the parsed JSON passed down to subsequent middlewares or the handler.

internal/models/api/queue.go Outdated Show resolved Hide resolved
internal/handler/queue.go Outdated Show resolved Hide resolved
internal/models/api/common.go Show resolved Hide resolved
internal/models/api/common.go Show resolved Hide resolved
internal/models/api/common.go Outdated Show resolved Hide resolved
@aneeshsharma
Copy link
Contributor Author

I started working on implementing a middleware to decode the JSON into our model. But kind of hit a roadblock. I don't think it is possible to implement a single function that can decode all requests into corresponding models since golang doesn't support generics.

Since we cannot decode the request into the API models, it is not possible to use the Validate function as it is right now.

I was thinking that we can instead use map objects as requests instead of models for all requests. Then inside the handler we can just extract data from that map. The validator can just check for the kind of request and validate according to that.

The other way would be to decode and validate inside each handler itself. Although I don't think it will be useful since we will have to implement a separate Validator for each request anyways.

I would say the map approach would make things more simple in all other areas except validation.

What do you think @daltonfury42 @maaverik ?

@daltonfury42
Copy link
Contributor

I see. Can I get on a call with you and then we can try the middleware approach together?

I see some support of generics, we can see if it will work: https://go.dev/doc/tutorial/generics

@aneeshsharma
Copy link
Contributor Author

As discussed with @daltonfury42 over call, we should go for decoding and validation of request body inside the handler itself as other approaches are unnecessarily complex specially given the small number of handlers.

@aneeshsharma aneeshsharma marked this pull request as ready for review February 1, 2022 07:54
maaverik
maaverik previously approved these changes Feb 2, 2022
Copy link
Contributor

@maaverik maaverik left a comment

Choose a reason for hiding this comment

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

Could you resolve the file conflicts pls?

@aneeshsharma
Copy link
Contributor Author

Resolved conflicts with main

@aneeshsharma aneeshsharma requested a review from maaverik February 4, 2022 04:30
maaverik
maaverik previously approved these changes Feb 6, 2022
Copy link
Contributor

@maaverik maaverik left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

@maaverik maaverik closed this Feb 6, 2022
@maaverik maaverik reopened this Feb 6, 2022
@maaverik
Copy link
Contributor

maaverik commented Feb 6, 2022

@daltonfury42 any idea why the status checks aren't running here?

@aneeshsharma
Copy link
Contributor Author

I was thinking it might be waiting for the 2 reviews to be completed before running checks.

@maaverik
Copy link
Contributor

maaverik commented Feb 7, 2022

I'm pretty sure that's not the issue, the checks normally run as soon as code is committed to the PR. I thought it might be due to a setting I had added to make sure the code is up-to-date with main on each PR, but I tried disabling that and reopened this PR (that shouldn't have caused a problem anyway since you merged with main), still nothing. I'll spend time later and try to figure it out, else we'll skip the checks this time.

daltonfury42
daltonfury42 previously approved these changes Feb 8, 2022
@daltonfury42
Copy link
Contributor

daltonfury42 commented Feb 8, 2022

My guess is that it will not run against these commits from other action. It's either a bug or some feature to stop infinite loop of commits or maybe a security feature for some sake.
image

@aneeshsharma can you try adding some empty/random commit to unblock.

Or maybe the commits by an action doesn't qualify as a push.

@aneeshsharma aneeshsharma dismissed stale reviews from daltonfury42 and maaverik via 84a9d55 February 9, 2022 04:10
@aneeshsharma
Copy link
Contributor Author

The checks work now. @daltonfury42 Can you please review the changes?

@aneeshsharma aneeshsharma merged commit b319914 into main Feb 11, 2022
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.

Have API Contract and Request Validation with proper error handling
4 participants