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

refactor(ValidateRequestBody): reduce cyclomatic complexity and indent #29

Merged
merged 2 commits into from
Nov 4, 2023

Conversation

nickajacks1
Copy link
Contributor

@nickajacks1 nickajacks1 commented Oct 1, 2023

Use the early-out pattern to reduce code complexity.
None of the existing logic should have changed, and all tests pass.

@daveshanley Can you fact check me at line 40? At this point, are there any errors that could occur here?

@codecov-commenter
Copy link

codecov-commenter commented Oct 7, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (affb03c) 100.00% compared to head (ada5f58) 100.00%.
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #29   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           18        18           
  Lines         1055      1071   +16     
=========================================
+ Hits          1055      1071   +16     
Flag Coverage Δ
unittests 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
requests/validate_body.go 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@daveshanley
Copy link
Member

A missing content type, I mean there is nothing to do, there is no work to perform, there is no way to know what to validate.

As soon as the coverage is back at 100%, I can merge this. I can't let coverage drop on this codebase, the tests are the guarantee it is as robust as we can make it.

@nickajacks1
Copy link
Contributor Author

nickajacks1 commented Oct 7, 2023

As soon as the coverage is back at 100%, I can merge this

Sure, I will take a crack at it.

From a branch coverage perspective though, the coverage remains identical since there is no logical change from the existing code. It's highly likely that there are more of these uncovered branches elsewhere in the code - for example, see validate_request.go. It also has a highly indented function that may or may not be missing test cases.
Going beyond line coverage may be effective in boosting the robustness of this library.

@nickajacks1
Copy link
Contributor Author

A missing content type, I mean there is nothing to do, there is no work to perform, there is no way to know what to validate.

Instantly passing validation is probably wrong as it might let an invalid body through to later handlers. I'm thinking it would be better to return errors.RequestContentTypeNotFound

@nickajacks1
Copy link
Contributor Author

By the way, I created TestValidateBody_MediaTypeHasNullSchema to reach 100% coverage, but it's technically testing an invalid openapi doc. A media type object must have a schema field (which can itself be empty, but then that doesn't cover the Schema == nil check).

@daveshanley daveshanley merged commit 91930e8 into pb33f:main Nov 4, 2023
1 check passed
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.

3 participants