Repository navigation
fix(graph): reject tag assignments that exceed the maximum - #3910
thelastbackspace wants to merge 1 commit into
Conversation
AssignTags passed the requested tags to reva's tags.Add, which stops at the maximum of 100 tags per resource without reporting it and only says whether at least one tag was added. So a request that didn't fully fit stored part of the tags and still returned 200, and the TagsAdded event listed every requested tag, including the dropped and already existing ones. Collect the distinct new tags first, reject the whole request with 400 unless all of them fit, and only publish the tags that were added. Fixes opencloud-eu#3893
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 20 |
🟢 Coverage 100.00% diff coverage · +0.06% coverage variation
Metric Results Coverage variation ✅ +0.06% coverage variation (-1.00%) Diff coverage ✅ 100.00% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (2eb513b) 93435 26771 28.65% Head commit (679a704) 93450 (+15) 26832 (+61) 28.71% (+0.06%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#3910) 17 17 100.00% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
pbleser-oc
left a comment
pbleser-oc
left a comment
There was a problem hiding this comment.
Thanks for picking this up, and for the tests.
The fix does what the issue describes: the whole request is rejected when the new tags don't all fit, and the event only carries the tags that were actually added.
Looks good to me. I've started the CI run.
Description
AssignTagspassed the requested tags straight to reva'stags.Add. That stops at 100 tags per resource without saying so and only returns whether at least one tag got added, so a request that only partly fit stored some of the tags and still returned 200.This collects the distinct new tags first (split on
,, empties and already existing tags skipped), and if they don't all fit the whole request is rejected with the existing 400 "no new tags in createtagsrequest or maximum reached", so nothing is stored partially. TheTagsAddedevent now only carries the tags that were actually added instead of the raw request list (which also included duplicates and tags the resource already had).Related Issue
Motivation and Context
Clients had no way to notice that tags were dropped, and event consumers saw tags that were never stored.
How Has This Been Tested?
services/graph/pkg/service/v0/tags_test.go:SetArbitraryMetadatanot called, no event (fails on main: the partial write goes through)a,b, requesta,c,c,d-> event hasc,d(main publishesa,c,c,d)go test ./services/graph/...passesTypes of changes
Checklist: