✨(ARSN-538) Accept undefined prefix for ReplicationConfiguration #2571

DarkIsDude · 2025-11-10T15:57:49Z

https://scality.atlassian.net/browse/ARSN-538

bert-e · 2025-11-10T15:57:52Z

Hello darkisdude,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Available options

name	description	privileged	authored
`/after_pull_request`	Wait for the given pull request id to be merged before continuing with the current one.
`/bypass_author_approval`	Bypass the pull request author's approval	⭐
`/bypass_build_status`	Bypass the build and test status	⭐
`/bypass_commit_size`	Bypass the check on the size of the changeset `TBA`	⭐
`/bypass_incompatible_branch`	Bypass the check on the source branch prefix	⭐
`/bypass_jira_check`	Bypass the Jira issue check	⭐
`/bypass_peer_approval`	Bypass the pull request peers' approval	⭐
`/bypass_leader_approval`	Bypass the pull request leaders' approval	⭐
`/approve`	Instruct Bert-E that the author has approved the pull request.		✍️
`/create_pull_requests`	Allow the creation of integration pull requests.
`/create_integration_branches`	Allow the creation of integration branches.
`/no_octopus`	Prevent Wall-E from doing any octopus merge and use multiple consecutive merge instead
`/unanimity`	Change review acceptance criteria from `one reviewer at least` to `all reviewers`
`/wait`	Instruct Bert-E not to run until further notice.

Available commands

name	description	privileged
`/help`	Print Bert-E's manual in the pull request.
`/status`	Print Bert-E's current status in the pull request `TBA`
`/clear`	Remove all comments from Bert-E from the history `TBA`
`/retry`	Re-start a fresh build `TBA`
`/build`	Re-start a fresh build `TBA`
`/force_reset`	Delete integration branches & pull requests, and restart merge process from the beginning.
`/reset`	Try to remove integration branches unless there are commits on them which do not appear on the source branch.

Status report is not available.

bert-e · 2025-11-10T15:58:00Z

Incorrect fix version

The Fix Version/s in issue ARSN-538 contains:

None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

8.2.38
8.3.0
8.4.0

Please check the Fix Version/s of ARSN-538, or the target
branch of this pull request.

codecov · 2025-11-10T16:07:52Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.40%. Comparing base (a000b51) to head (aec1b5a).
⚠️ Report is 1 commits behind head on development/8.2.

Additional details and impacted files

@@               Coverage Diff                @@
##           development/8.2    #2571   +/-   ##
================================================
  Coverage            71.40%   71.40%           
================================================
  Files                  221      221           
  Lines                17816    17817    +1     
  Branches              3705     3706    +1     
================================================
+ Hits                 12721    12723    +2     
+ Misses                5091     5090    -1     
  Partials                 4        4

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

SylvainSenechal · 2025-11-10T16:31:48Z

tests/unit/models/ReplicationConfiguration.spec.ts

        expect(rules[0].prefix).toEqual('');
    });

+    it('should succeed for a valid configuration without a prefix', () => {


Add William as a reviewer as it's touching the db, I'm not too sure I can review confidently.
Also, is it documented on aws that replication configurations may not have a prefix anymore ? Can it impact some logic elsewhere if prefix is sometimes removed ?

I don't think so, with Typescript I hope we are safe in Arsenal 🙏

francoisferrand · 2025-11-12T11:35:32Z

lib/models/ReplicationConfiguration.ts

-        if (!prefix && prefix !== '') {
+        const prefix = rule.Prefix?.[0] ?? '';
+
+        if (typeof prefix !== 'string') {


when can this happen?
rule.Prefix is types as optional string[], then we it gets a default if null/undefined : so it may only be a string....

Maybe the typing is not the right one, but prefix can be an array, an object. Indeed, at this step we just parsed the XML without any validation. That's why we have this check. Does it make sense ?

Indeed no need to check for the type here , can only be a string if it's not undefined/null

@benzekrimaha are you double sure ? This is accepted with the current version:

<?xml version="1.0" encoding="UTF-8"?> <ReplicationConfiguration xmlns="http://s3.amazonaws.com/doc/2006-03-01/"> <Role>arn:aws:iam::003641996138:role/scality-internal/storage-manager-role</Role> <Role>arn:aws:iam::003641996138:role/scality-internal/storage-manager-role</Role> <Rule> <ID>testfdg</ID> <Prefix> <Test>OUI</Test> </Prefix> <Filter> <Prefix>fd</Prefix> </Filter> <Status>Enabled</Status> <Destination> <Bucket>arn:aws:s3:::aa-bb-2</Bucket> </Destination> </Rule> </ReplicationConfiguration>

When I guess it should not. That's why we have this check (you can check new tests).

Side question, with this request, would (and should) prefix be equal to String('OUI') ?

Also other side question, why is XMLRule.Prefix an array ? It doesn't make much sense, on top of that we see in the function that we are getting the first element only. What am I missing 🤔

That good questions ! For the first one I tried with AWS and the request is rejected in that case, so I guess the answer is no, the request should be rejected (what I did). The second part it's how the xml parser works. In that case for example the Test will be an array of length 1 and a string OUI. Should I add one more check to make sure const prefix length is one and only one ?

Test is up to you, but no need if you wanna merge now.
Ah yeah the xml parser parse everything as an array

francoisferrand · 2025-11-12T11:41:29Z

tests/unit/models/ReplicationConfiguration.spec.ts

+        expect(typeof rules[0].id).toBe('string');
+        expect(rules[0].prefix).toEqual('');
+    });
+


Should have a test validating that we return malformed xml when the "prefix is invalid" (whatever it means) - or return that test if not relevant anymore (c.f. earlier comment).

the expression used to get prefix in _parsePrefix (and later malformed xml condition) is not trivial, would be better to make this plain with unit tests for _parsePrefix : which show exactly what "works" and what does not, and covers the 4 code-path (I think) in

const prefix = rule.Prefix?.[0] ?? ''; if (typeof prefix !== 'string') {

Make sense 🙏. I added them

francoisferrand · 2025-11-12T11:46:07Z

lib/models/ReplicationConfiguration.ts

        const base = {
            id: '',
-            prefix: rule.Prefix[0],
+            prefix: rule.Prefix?.[0] ?? '',


this is duplicated with _parsePrefix, can we avoid this and do the "parsing" just once?

(though parsePrefix name is actually incorrect, it really just validates the prefix...)

The caller of _buildRuleObject is already doing all the parsing verifications. I think it would be complicated to use _parsePrefix in this function, as it would now have to return an error.

What I would do is rename the rule parameter like this :

Suggested change

prefix: rule.Prefix?.[0] ?? '',

_buildRuleObject(rule: XMLRule) {

_buildRuleObject(validRule: XMLRule) {

bert-e · 2025-11-12T11:48:11Z

Request integration branches

Waiting for integration branch creation to be requested by the user.

To request integration branches, please comment on this pull request with the following command:

/create_integration_branches

Alternatively, the /approve and /create_pull_requests commands will automatically
create the integration branches.

SylvainSenechal

approved but review the few comments from the others

DarkIsDude added 2 commits November 10, 2025 16:56

✨(ARSN-538) accept empty prefix for ReplicationConfiguration

d1b77a0

🔖 increase Arsenal version

31ab345

DarkIsDude self-assigned this Nov 10, 2025

DarkIsDude changed the title ~~✨(ARSN-538) Accept undefine prefix for ReplicationConfiguration~~ ✨(ARSN-538) Accept undefined prefix for ReplicationConfiguration Nov 10, 2025

DarkIsDude requested review from a team, SylvainSenechal and francoisferrand November 10, 2025 15:58

💡(ARSN-538) remove useless comment in the code

a44dad8

SylvainSenechal reviewed Nov 10, 2025

View reviewed changes

DarkIsDude requested a review from williamlardier November 12, 2025 09:50

francoisferrand reviewed Nov 12, 2025

View reviewed changes

francoisferrand approved these changes Nov 12, 2025

View reviewed changes

✅ add tests on the prefix and use one describe

aec1b5a

DarkIsDude requested review from SylvainSenechal, benzekrimaha and francoisferrand November 12, 2025 14:48

SylvainSenechal approved these changes Nov 13, 2025

View reviewed changes

	prefix: rule.Prefix?.[0] ?? '',
	_buildRuleObject(rule: XMLRule) {
	_buildRuleObject(validRule: XMLRule) {

✨(ARSN-538) Accept undefined prefix for ReplicationConfiguration #2571

Are you sure you want to change the base?

✨(ARSN-538) Accept undefined prefix for ReplicationConfiguration #2571

Conversation

DarkIsDude commented Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bert-e commented Nov 10, 2025

Hello darkisdude,

Uh oh!

bert-e commented Nov 10, 2025

Incorrect fix version

Uh oh!

codecov bot commented Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bert-e commented Nov 12, 2025

Request integration branches

Uh oh!

SylvainSenechal left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

DarkIsDude commented Nov 10, 2025 •

edited

Loading

codecov bot commented Nov 10, 2025 •

edited

Loading