RHINENG-22863: limit systems to 1000 in update and delete template system apis #2031
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Referenced Jiras: |
Reviewer's GuideAdds a hard limit of 1000 systems per request to the template systems update and delete APIs and validates incoming payloads against this limit, returning a 400 error with a clear message when exceeded, along with tests for the new behavior. Sequence diagram for template systems update with max-systems validationsequenceDiagram
actor Client
participant GinRouter
participant TemplateSystemsUpdateHandler
participant Utils
participant Candlepin
Client->>GinRouter: HTTP PATCH /templates/{id}/systems
GinRouter->>TemplateSystemsUpdateHandler: TemplateSystemsUpdateHandler(c)
TemplateSystemsUpdateHandler->>TemplateSystemsUpdateHandler: ParseAndValidateRequestBody
TemplateSystemsUpdateHandler->>TemplateSystemsUpdateHandler: Check account and groups
TemplateSystemsUpdateHandler->>TemplateSystemsUpdateHandler: len(req.Systems) > TemplateSystemsUpdateLimit?
alt Over_limit
TemplateSystemsUpdateHandler->>Utils: LogAndRespBadRequest(c, err, msg)
Utils-->>Client: HTTP 400 Cannot update more than 1000 systems at once
else Within_limit
TemplateSystemsUpdateHandler->>TemplateSystemsUpdateHandler: DBFromContext(c)
TemplateSystemsUpdateHandler->>Candlepin: Update template systems
Candlepin-->>TemplateSystemsUpdateHandler: Success
TemplateSystemsUpdateHandler-->>Client: HTTP 200 OK
end
Sequence diagram for template systems delete with max-systems validationsequenceDiagram
actor Client
participant GinRouter
participant TemplateSystemsDeleteHandler
participant Utils
participant Candlepin
Client->>GinRouter: HTTP DELETE /templates/{id}/systems
GinRouter->>TemplateSystemsDeleteHandler: TemplateSystemsDeleteHandler(c)
TemplateSystemsDeleteHandler->>TemplateSystemsDeleteHandler: ParseAndValidateRequestBody
TemplateSystemsDeleteHandler->>TemplateSystemsDeleteHandler: Check account and groups
TemplateSystemsDeleteHandler->>TemplateSystemsDeleteHandler: len(req.Systems) > TemplateSystemsUpdateLimit?
alt Over_limit
TemplateSystemsDeleteHandler->>Utils: LogAndRespBadRequest(c, err, msg)
Utils-->>Client: HTTP 400 Cannot update more than 1000 systems at once
else Within_limit
TemplateSystemsDeleteHandler->>TemplateSystemsDeleteHandler: DBFromContext(c)
TemplateSystemsDeleteHandler->>Candlepin: Remove systems from template
Candlepin-->>TemplateSystemsDeleteHandler: Success
TemplateSystemsDeleteHandler-->>Client: HTTP 200 OK
end
Updated class diagram for template systems controllers with max-systems limitclassDiagram
class TemplateSystemsUpdateRequest {
+[]string Remove
+[]string Apply
+[]string Systems
}
class TemplateSystemsUpdateHandlerController {
+TemplateSystemsUpdateHandler(c *gin.Context)
+int TemplateSystemsUpdateLimit
}
class TemplateSystemsDeleteHandlerController {
+TemplateSystemsDeleteHandler(c *gin.Context)
}
class Utils {
+LogAndRespBadRequest(c *gin.Context, err error, msg string)
}
TemplateSystemsUpdateHandlerController --> TemplateSystemsUpdateRequest : uses
TemplateSystemsUpdateHandlerController --> Utils : calls
TemplateSystemsDeleteHandlerController --> Utils : calls
TemplateSystemsDeleteHandlerController --> TemplateSystemsUpdateHandlerController : uses_TemplateSystemsUpdateLimit
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The over-limit check and error message are duplicated between TemplateSystemsUpdateHandler and TemplateSystemsDeleteHandler; consider extracting a small shared helper (or shared error message constant) to keep behavior and wording in sync.
- The error message uses "update" even in the delete handler and its tests; consider rephrasing to something operation-agnostic like "process" or using separate messages for update vs delete to avoid confusing API consumers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The over-limit check and error message are duplicated between TemplateSystemsUpdateHandler and TemplateSystemsDeleteHandler; consider extracting a small shared helper (or shared error message constant) to keep behavior and wording in sync.
- The error message uses "update" even in the delete handler and its tests; consider rephrasing to something operation-agnostic like "process" or using separate messages for update vs delete to avoid confusing API consumers.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2031 +/- ##
==========================================
+ Coverage 59.25% 59.30% +0.05%
==========================================
Files 134 134
Lines 8615 8626 +11
==========================================
+ Hits 5105 5116 +11
Misses 2967 2967
Partials 543 543
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
MichaelMraka
requested changes
Jan 28, 2026
Collaborator
MichaelMraka
left a comment
MichaelMraka
left a comment
There was a problem hiding this comment.
Thanks and congratulation on your first PR to Patch. ;)
| err := errors.New(msg) | ||
| utils.LogAndRespBadRequest(c, err, msg) | ||
| return | ||
| } |
Collaborator
There was a problem hiding this comment.
I think sourcery has a good point this could be a small helper function to keep wording an behavior consitent, e.g.
func CheckTemplateSystemsLimit(nSystems int) err {
if len(nSystems) > TemplateSystemsUpdateLimit {
msg := fmt.Sprintf("Cannot update more than %d systems at once", TemplateSystemsUpdateLimit)
err := errors.New(msg)
utils.LogAndRespBadRequest(c, err, msg)
return err
}
return nil
}
and then in code
if err = CheckTemplateSystemsLimit(len(req.Systems)); err != nil {
// result code handeled in CheckTemplateSystemsLimit()
return
}
Collaborator
There was a problem hiding this comment.
And possibly 'Cannot update' could be changed to some neutral wording which fits both update and delete. Maybe 'Cannot process'?!?
Author
There was a problem hiding this comment.
oh yes this is better, thank you! updated :)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Updating or deleting a template from more than 1000 systems at one time results in a candlepin error, likely because we are hitting a defined limit:
This PR limits the number of requested systems to 1000 in the update and delete template system apis.
Summary by Sourcery
Enforce a maximum number of systems that can be updated or removed from a template in a single API call to prevent downstream Candlepin errors.
Bug Fixes:
Enhancements: