Skip to content

Add support for S3 Multi-Region Access Point (MRAP) URLs#557

Merged
pjbull merged 10 commits intodrivendataorg:557-live-testsfrom
fedemengo:feat/support-s3-mrap
Apr 29, 2026
Merged

Add support for S3 Multi-Region Access Point (MRAP) URLs#557
pjbull merged 10 commits intodrivendataorg:557-live-testsfrom
fedemengo:feat/support-s3-mrap

Conversation

@fedemengo
Copy link
Copy Markdown
Contributor

@fedemengo fedemengo commented Apr 9, 2026

Closes #556

Changes

  • cloudpathlib/s3/s3path.py: added _MRAP_PATTERN regex and overridden bucket property on S3Path
  • tests/mock_clients/mock_s3.py: removed bucket name check from head_object (the mock never routes by bucket, only by key)
  • tests/test_s3_specific.py: added 3 tests covering bucket/key parsing, path manipulation, and end-to-end file operations via the
    mock backend

Note: using MRAP paths requires botocore[crt] for SigV4a signing:

pip install botocore[crt]


Contributor checklist:

  • I have read and understood CONTRIBUTING.md
  • Confirmed an issue exists for the PR, and the text Closes #issue appears in the PR summary (e.g., Closes #123).
  • Confirmed PR is rebased onto the latest base
  • Confirmed failure before change and success after change
  • Any generic new functionality is replicated across cloud providers if necessary
  • Tested manually against live server backend for at least one provider
  • Added tests for any new functionality
  • Linting passes locally
  • Tests pass locally
  • Updated HISTORY.md with the issue that is addressed and the PR you are submitting. If the top section is not `## UNRELEASED``, then you need to add a new section to the top of the document for your change.

@fedemengo
Copy link
Copy Markdown
Contributor Author

the changes were simple enough, @pjbull gentle ping for review

Copy link
Copy Markdown
Member

@pjbull pjbull left a comment

Choose a reason for hiding this comment

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

A couple comments.Also is it possible to update the tests to use s3_rig following the pattern of the existing tests rather than using S3Path directly (as in some tests) or using monkeypatch as in the others.

Comment thread cloudpathlib/s3/s3path.py Outdated
:type: :class:`str`
"""
if match := _MRAP_PATTERN.match(str(self)):
return match.group("arn")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's cache this since it shouldn't change.

if (
not (self.root / Key).exists()
or (self.root / Key).is_dir()
or Bucket != DEFAULT_S3_BUCKET_NAME
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

IIRC some of the tests use this check in some scenarios. Can we also just add a simple check that it doesn't have '.mrap' here

@fedemengo fedemengo force-pushed the feat/support-s3-mrap branch from 3106e54 to b86732d Compare April 17, 2026 15:52
@fedemengo fedemengo requested a review from pjbull April 17, 2026 15:56
@fedemengo
Copy link
Copy Markdown
Contributor Author

@pjbull I replaced S3Path with rig_ops when tests are not purly path parsing

@pjbull
Copy link
Copy Markdown
Member

pjbull commented Apr 23, 2026

@fedemengo see linting errors in the action logs.

@fedemengo
Copy link
Copy Markdown
Contributor Author

@fedemengo see linting errors in the action logs.

@pjbull I had missed that 8daee30

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 24, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.2%. Comparing base (5124aa0) to head (ac20cb4).
⚠️ Report is 1 commits behind head on 557-live-tests.

Additional details and impacted files
@@               Coverage Diff                @@
##           557-live-tests    #557     +/-   ##
================================================
- Coverage            94.0%   93.2%   -0.9%     
================================================
  Files                  28      28             
  Lines                2203    2222     +19     
================================================
  Hits                 2071    2071             
- Misses                132     151     +19     
Files with missing lines Coverage Δ
cloudpathlib/s3/s3path.py 100.0% <100.0%> (ø)

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@pjbull
Copy link
Copy Markdown
Member

pjbull commented Apr 24, 2026

@fedemengo tests are failing on windows because : is not a valid character in the filepath.

We're coming up against this separate issue:
#439

Maybe for now you can override the _local property on S3Path and if there is a : and we are on Windows, we percent-encode the :?

@fedemengo fedemengo force-pushed the feat/support-s3-mrap branch from 8089276 to baf833e Compare April 27, 2026 10:13
@fedemengo
Copy link
Copy Markdown
Contributor Author

@pjbull something like this baf833e

@pjbull
Copy link
Copy Markdown
Member

pjbull commented Apr 28, 2026

Test needs an update to strip .drive from the path before checking no ::.

FAILED tests/test_s3_specific.py::test_mrap_local_path_windows_encoding - AssertionError: Colon found in local path on simulated Windows: C:\Users\RUNNER~1\AppData\Local\Temp\tmpc_bmszu4\arn%3Aaws%3As3%3A%3A123456789012%3Aaccesspoint\my-mrap.mrap\some\key.txt
assert ':' not in 'C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\tmpc_bmszu4\\arn%3Aaws%3As3%3A%3A123456789012%3Aaccesspoint\\my-mrap.mrap\\some\\key.txt'

@pjbull pjbull mentioned this pull request Apr 28, 2026
@fedemengo
Copy link
Copy Markdown
Contributor Author

@pjbull ok hopefully this is it, if there is another failure I'll probably spin up a Windows vm

@pjbull
Copy link
Copy Markdown
Member

pjbull commented Apr 29, 2026

@fedemengo linting failure

@fedemengo
Copy link
Copy Markdown
Contributor Author

@pjbull my bad, ran the linter ac20cb4

@pjbull pjbull changed the base branch from master to 557-live-tests April 29, 2026 18:57
@pjbull pjbull merged commit dd966b3 into drivendataorg:557-live-tests Apr 29, 2026
26 of 27 checks passed
pjbull added a commit that referenced this pull request Apr 29, 2026
* Add support for S3 Multi-Region Access Point (MRAP) URLs (#557)

* Add MRAP URL support

* test MRAP

* update history

* use walrus operator

* cache bucket parsing

* address pr review

* fix linter issues

* url escape `:` on win and cache

* handle Windows drive in test assertion

* make lint

* fix live tests with live mrap

* prep for release

* Keep CRT optional

---------

Co-authored-by: Federico Mengozzi <19249682+fedemengo@users.noreply.github.com>
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.

Add support for S3 Multi-Region Access Point (MRAP) URLs

2 participants