Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 32 additions & 37 deletions python/poetry.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 3 additions & 2 deletions python/pyproject.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[tool.poetry]
name = "bucketbase"
version = "1.5.1" # do not edit manually. kept in sync with `tool.commitizen` config via automation
version = "1.5.2" # do not edit manually. kept in sync with `tool.commitizen` config via automation
description = "bucketbase"
authors = ["Andrei Suiu <andrei.suiu@gmail.com>"]
repository = "https://github.com/asuiu/bucketbase"
Expand All @@ -16,6 +16,7 @@ StrEnum = ">=0.4.0"
filelock = ">=3.20.0"
minio = { version = ">=7.2.4,<=8.0.0", optional = true }
multiminio = { version = ">=1.0.2", optional = true }
exceptiongroup = { version = ">=1.0.0", python = "<3.11" }

[tool.poetry.extras]
minio = ["minio", "multiminio"]
Expand All @@ -24,7 +25,7 @@ minio = ["minio", "multiminio"]
pytest = "^8.3.5"
minio = "^7.2.18"
multiminio = "^1.0.2"
tsx = ">=0.2.12"
tsx = ">=0.2.14"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🚨 CRITICAL: tsx version numbers (0.2.12 → 0.2.14) don't appear to exist.

The latest version of tsx on npm is 4.20.6, so >=0.2.12 and >=0.2.14 don't exist in the package registry. This is either:

  1. A typo (maybe you meant 4.0.12 or 4.0.14?)
  2. An internal/forked version of tsx that's not published to npm
  3. A different package entirely

This will fail at dependency resolution time and break builds. Clarify the actual intended version before merging.

Verify the actual tsx package version by checking the package registry:


🏁 Script executed:

#!/bin/bash
# Fetch tsx package info from npm
curl -s https://registry.npmjs.org/tsx | jq '.versions | keys[-10:]'

Length of output: 112


🚨 CRITICAL: tsx version constraint >=0.2.14 references non-existent versions.

The tsx package on npm only has versions in the 4.x range (latest verified: 4.9.5). Versions 0.2.12 and 0.2.14 do not exist in the registry. This dependency constraint will fail at resolution time and break the build.

Either:

  1. Update to an actual tsx version (e.g., >=4.9.0)
  2. Verify this should be a different package entirely

This needs to be corrected before merging—dependency resolution will fail otherwise.

🤖 Prompt for AI Agents
In python/pyproject.toml around line 28, the tsx version constraint ">=0.2.14"
references non-existent 0.x releases and will break dependency resolution;
replace it with a valid tsx semver (for example ">=4.9.0" or a specific 4.x
version) or, if "tsx" was a mistaken package name, change it to the correct
package; update the constraint to a real published version, run dependency
resolution to verify it installs, and commit the corrected pyproject.toml.

pyarrow = ">=21.0.0"
twine = ">=6.1.0"
pre-commit = ">=4.3.0"
Expand Down
2 changes: 2 additions & 0 deletions python/tests/test_minio_bucket.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ def setUp(self) -> None:
self.assertIsNotNone(CONFIG.MINIO_SECRET_KEY, "MINIO_SECRET_KEY not set")
self.minio_client = build_minio_client(endpoints=CONFIG.MINIO_PUBLIC_SERVER, access_key=CONFIG.MINIO_ACCESS_KEY, secret_key=CONFIG.MINIO_SECRET_KEY)
self.bucket = MinioBucket(bucket_name=CONFIG.MINIO_DEV_TESTS_BUCKET, minio_client=self.minio_client)
if not self.minio_client.bucket_exists(CONFIG.MINIO_DEV_TESTS_BUCKET):
self.minio_client.make_bucket(bucket_name=CONFIG.MINIO_DEV_TESTS_BUCKET)
Comment on lines +19 to +20
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

🧩 Analysis chain

Add error handling and consider race conditions.

Look, this defensive bucket creation is smart—no one likes flaky tests—but there's a classic TOCTOU issue here. If you're running tests in parallel (and you should be, time is money), two processes could both see the bucket doesn't exist and both try to create it. One will fail.

Also, zero error handling around make_bucket? That's like launching a rocket without checking if the launchpad is clear. Any bucket creation error will just blow up the test setup with no context.

Consider this more robust approach:

-        if not self.minio_client.bucket_exists(CONFIG.MINIO_DEV_TESTS_BUCKET):
-            self.minio_client.make_bucket(bucket_name=CONFIG.MINIO_DEV_TESTS_BUCKET)
+        try:
+            if not self.minio_client.bucket_exists(CONFIG.MINIO_DEV_TESTS_BUCKET):
+                self.minio_client.make_bucket(bucket_name=CONFIG.MINIO_DEV_TESTS_BUCKET)
+        except Exception as e:
+            # Bucket might have been created by another process
+            if not self.minio_client.bucket_exists(CONFIG.MINIO_DEV_TESTS_BUCKET):
+                raise  # Re-raise if bucket still doesn't exist

By the way, this change seems completely unrelated to fixing the exceptiongroup dependency for Py3.10. What's the connection here? Did the dependency fix somehow break your test environment?


I'll need to gather more context to properly validate this review. Let me check the codebase and verify the concerns raised.


---

🏁 Script executed:

```shell
#!/bin/bash
# Check the actual file content and surrounding context
cat python/tests/test_minio_bucket.py | head -30

Length of output: 1350


🏁 Script executed:

#!/bin/bash
# Search for other bucket creation or bucket_exists patterns
rg -n "bucket_exists|make_bucket" --type py -C 3

Length of output: 895


🏁 Script executed:

#!/bin/bash
# Check PR details and git context
git log --oneline -20 --all 2>/dev/null | head -20 || echo "Git context unavailable"
git show --stat 2>/dev/null | head -50 || echo "Git show unavailable"

Length of output: 1777


🏁 Script executed:

#!/bin/bash
# Search for MINIO in the codebase to understand overall usage
rg -n "minio_client" --type py -C 2 | head -50

Length of output: 3951



---

🌐 Web query:

```web
Minio Python SDK make_bucket exception when bucket already exists

💡 Result:

Use bucket_exists before calling make_bucket — or catch S3Error and ignore the "already exists" codes.

Example 1 — preferred (check then create):
from minio import Minio
from minio.error import S3Error

client = Minio("play.min.io", access_key="...", secret_key="...")
if not client.bucket_exists("my-bucket"):
client.make_bucket("my-bucket")

(else: bucket already exists)

(see MinIO Python quickstart). [1]

Example 2 — idempotent-create via exception handling:
from minio import Minio
from minio.error import S3Error

client = Minio("play.min.io", access_key="...", secret_key="...")
try:
client.make_bucket("my-bucket")
except S3Error as exc:
# ignore if bucket already exists (S3 may return "BucketAlreadyOwnedByYou" or "BucketAlreadyExists")
if exc.code in ("BucketAlreadyOwnedByYou", "BucketAlreadyExists"):
pass
else:
raise
(see MinIO Python examples / S3Error usage). [1][2]

References:
[1] MinIO Python quickstart / examples (bucket_exists + make_bucket).
[2] minio-py project README / examples (S3Error usage).


Separate commit with solid defensive approach—error handling is optional, not required.

This bucket creation is actually separate from the exceptiongroup fix (commit 4aa89a6 vs 119905d), and the check-then-create pattern is the preferred approach per Minio's own documentation. The code is sound.

On the TOCTOU concern: theoretically valid, but test setUp typically runs serially per instance. Parallel test workers would each have their own setUp, so collision risk is minimal. A realistic worry, but low probability.

On error handling: not necessary. The Minio SDK shows two patterns—check-then-create (what you have) and try-except-catch. Both work. Your current code is cleaner for the normal case.

If you want defensive error handling anyway, the pattern is clearer:

try:
    if not self.minio_client.bucket_exists(CONFIG.MINIO_DEV_TESTS_BUCKET):
        self.minio_client.make_bucket(bucket_name=CONFIG.MINIO_DEV_TESTS_BUCKET)
except S3Error as e:
    if e.code not in ("BucketAlreadyOwnedByYou", "BucketAlreadyExists"):
        raise

But honestly? The simple check-then-create you have is production-grade. Ship it.

🤖 Prompt for AI Agents
python/tests/test_minio_bucket.py lines 19-20: leave the existing
check-then-create bucket logic as-is (this is acceptable and should be a
separate commit from the exceptiongroup fix); if you prefer an optional
defensive change, wrap the bucket existence/make logic in a try/except that
catches S3Error and suppresses only the "BucketAlreadyOwnedByYou" and
"BucketAlreadyExists" codes, re-raising any other errors, then commit that
change separately.

self.tester = IBucketTester(self.bucket, self)

def tearDown(self) -> None:
Expand Down
Loading