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(webapp): add go controller for cves endpoint #1193

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Dugowitch
Copy link

@Dugowitch Dugowitch commented Nov 15, 2024

Secure Coding Practices Checklist GitHub Link

Secure Coding Checklist

  • Input Validation
  • Output Encoding
  • Authentication and Password Management
  • Session Management
  • Access Control
  • Cryptographic Practices
  • Error Handling and Logging
  • Data Protection
  • Communication Security
  • System Configuration
  • Database Security
  • File Management
  • Memory Management
  • General Coding Practices

Copy link

jira-linking bot commented Nov 15, 2024

Referenced Jiras:
https://issues.redhat.com/browse/RHINENG-13545

@codecov-commenter
Copy link

codecov-commenter commented Nov 15, 2024

Codecov Report

Attention: Patch coverage is 7.89474% with 35 lines in your changes missing coverage. Please review.

Project coverage is 67.70%. Comparing base (5a56460) to head (8744de0).
Report is 152 commits behind head on master.

Files with missing lines Patch % Lines
vmaas-go/webapp/controllers/cves.go 0.00% 29 Missing ⚠️
vmaas-go/webapp/controllers/utils.go 33.33% 4 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1193      +/-   ##
==========================================
+ Coverage   62.98%   67.70%   +4.71%     
==========================================
  Files          70       74       +4     
  Lines        6800     6127     -673     
==========================================
- Hits         4283     4148     -135     
+ Misses       2517     1976     -541     
- Partials        0        3       +3     

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

@Dugowitch Dugowitch requested a review from psegedy November 15, 2024 18:01
deploy/clowdapp.yaml Outdated Show resolved Hide resolved
vmaas-go/webapp/controllers/cves.go Outdated Show resolved Hide resolved
@Dugowitch Dugowitch force-pushed the cves-controller branch 3 times, most recently from 588b16e to 8fa2e3d Compare November 18, 2024 16:32
@Dugowitch Dugowitch requested a review from psegedy November 18, 2024 17:08
Copy link
Member

@psegedy psegedy left a comment

Choose a reason for hiding this comment

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

looks good. Please make a MR to app-interface enabling this API so we can see PR check test results.

@psegedy
Copy link
Member

psegedy commented Nov 19, 2024

/retest

vmaas-go/webapp/routes/routes.go Outdated Show resolved Hide resolved
@psegedy
Copy link
Member

psegedy commented Nov 20, 2024

/retest

@Dugowitch Dugowitch marked this pull request as draft November 29, 2024 16:55
@Dugowitch Dugowitch force-pushed the cves-controller branch 2 times, most recently from a3bb2a3 to 537408b Compare December 2, 2024 14:52
@Dugowitch Dugowitch requested a review from psegedy December 2, 2024 15:15
@Dugowitch Dugowitch marked this pull request as ready for review December 2, 2024 15:15
@psegedy
Copy link
Member

psegedy commented Dec 9, 2024

/retest

@Dugowitch Dugowitch changed the title Add go controller for cves endpoint feat(webapp): add go controller for cves endpoint Dec 11, 2024
@Dugowitch Dugowitch marked this pull request as draft December 11, 2024 12:10
@Dugowitch Dugowitch marked this pull request as ready for review December 13, 2024 09:39
Comment on lines +109 to +127
func processBadRequestErrMessage(err error) error {
errMessage := err.Error()
if strings.HasPrefix(errMessage, "parsing time") {
parts := strings.Split(errMessage, `"`)
if len(parts) < 2 {
return errors.New("Wrong date format (not ISO format with timezone)")
}
return errors.New("Wrong date format (not ISO format with timezone): " + parts[1])
}
if strings.HasSuffix(errMessage, "looking for beginning of value") {
parts := strings.Split(errMessage, ` `)
if len(parts) < 3 {
return errors.New("malformed input")
}
return errors.New("malformed input: invalid character " + parts[2])
}
return err
}

Copy link
Member

Choose a reason for hiding this comment

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

Parsing errors based on error message and then changing the error message is not optimal. I'd prefer wrapping original error.
Error for wrong date can be for example handled when UnmarshalJSON for time fails (it would require custom type for time so UnmarshalJSON can be defined, something like https://github.com/RedHatInsights/patchman-engine/blob/master/base/types/timestamp.go). Anyways, timestamp is parsed only by bindValidateJSON so at least move the current logic to that function.

looking for beginning of value is returned by regex error? In that case you can wrap the error in vmaas-lib before returning regex error or return some custom error type and then change the message after checking the error type with errors.Is

Copy link
Author

Choose a reason for hiding this comment

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

The original error parsing time "2017-01-01T00:00:00" as "2006-01-02T15:04:05Z07:00": cannot parse "" as "Z07:00" is clunky and requires some go-specific knowledge to understand. That's why I opted to replace it rather than to just wrap it.

invalid character 'N' looking for beginning of value also occurs during JSON parsing, for example, when there is NaN instead of a number.

I will move the logic. Should I also wrap the errors instead?

Comment on lines +112 to +116
parts := strings.Split(errMessage, `"`)
if len(parts) < 2 {
return errors.New("Wrong date format (not ISO format with timezone)")
}
return errors.New("Wrong date format (not ISO format with timezone): " + parts[1])
Copy link
Member

Choose a reason for hiding this comment

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

what's the purpose of splitting the error message? When it has only 1 " then the error will be Wrong date format (not ISO format with timezone and when there are more " then the second part is wrapped by the new message and other parts are discarded?
If the point is to only wrap the current error, you can use errors.Wrap from github.com/pkg/errors which is already used in this module or https://pkg.go.dev/errors#Join

Copy link
Author

Choose a reason for hiding this comment

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

The purpose is to get the original timestamp being parsed, the error message without one is just a fallback.

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