-
Notifications
You must be signed in to change notification settings - Fork 354
Fix for v7.2.19 related issues #1535
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request removes keyword-only argument restrictions (the * separator) from most function signatures in the MinIO Python SDK and introduces new parameters to improve API usability. The changes convert many previously keyword-only parameters to positional-or-keyword parameters while strategically keeping some parameters keyword-only for backward compatibility.
Key Changes:
- Removed bare
*separators from function signatures across multiple modules, allowing parameters to be passed positionally - Added new
metadataparameter to several functions (fput_object,put_object,copy_object,compose_object,upload_snowball_objects) as an alternative touser_metadata - Added new functionality parameters:
write_offsetinput_object,request_headersinget_object/fget_object/prompt_object, andresponse_headersin presigned URL functions - Reordered parameters in several functions to improve API ergonomics
- Updated documentation to reflect signature changes (though with some inconsistencies)
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| minio/signer.py | Removed keyword-only restrictions from signature verification functions |
| minio/minioadmin.py | Removed keyword-only restrictions from MinioAdmin class constructor and methods |
| minio/helpers.py | Removed keyword-only restrictions from helper functions and BaseURL methods |
| minio/credentials/providers.py | Removed keyword-only restrictions from credential provider constructors |
| minio/api.py | Major changes: removed keyword-only restrictions, added new parameters (metadata, write_offset, request_headers, response_headers), reordered parameters for better API design. Contains several bugs including duplicated code and missing metadata handling |
| docs/API.md | Updated API documentation signatures to remove keyword-only markers, though documentation is incomplete and missing several new parameters |
Comments suppressed due to low confidence (1)
docs/API.md:2260
- Missing parameter in documentation. The function signature now includes a
metadataparameter (see api.py line 5367), but this is not documented. Additionally, the documentation should reflect thatheadersanduser_metadataare now keyword-only parameters (after the*separator).
### upload_snowball_objects(self, bucket_name: str, objects: Iterable[SnowballObject], headers: Optional[HTTPHeaderDict] = None, user_metadata: Optional[HTTPHeaderDict] = None, sse: Optional[Sse] = None, tags: Optional[Tags] = None, retention: Optional[Retention] = None, legal_hold: bool = False, staging_filename: Optional[str] = None, compression: bool = False, region: Optional[str] = None, extra_headers: Optional[HTTPHeaderDict] = None, extra_query_params: Optional[HTTPQueryDict] = None) -> ObjectWriteResult
Uploads multiple objects in a single put call. It is done by creating intermediate TAR file optionally compressed which is uploaded to S3 service.
__Parameters__
| Param | Type | Description |
|:---------------------|:------------------------------------------------|:---------------------------------------------------|
| `bucket_name` | _str_ | Name of the bucket. |
| `objects` | _Iterable[minio.commonconfig.SnowballObject]_ | An iterable contain snowball object. |
| `headers` | _Optional[minio.helpers.HTTPHeaderDict] = None_ | Additional headers. |
| `user_metadata` | _Optional[minio.helpers.HTTPHeaderDict] = None_ | User metadata. |
| `sse` | _Optional[minio.sse.Sse] = None_ | Server-side encryption. |
| `tags` | _Optional[minio.commonconfig.Tags] = None_ | Tags for the object. |
| `retention` | _Optional[minio.retention.Retention] = None_ | Retention configuration. |
| `legal_hold` | _bool = False_ | Flag to set legal hold for the object. |
| `staging_filename` | _Optional[str] = None_ | A staging filename to create intermediate tarball. |
| `compression` | _bool = False_ | Flag to compress tarball. |
| `region` | _Optional[str] = None_ | Region of the bucket to skip auto probing. |
| `extra_headers` | _Optional[minio.helpers.HTTPHeaderDict] = None_ | Extra headers for advanced usage. |
| `extra_query_params` | _Optional[minio.helpers.HTTPQueryDict] = None_ | Extra query parameters for advanced usage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
docs/API.md:1529
- The parameter table order doesn't match the function signature order. The parameters should be reordered to match the signature, grouping positional parameters before the keyword-only separator (
*,) and keyword-only parameters after it.
Based on the signature, the table should list parameters in this order:
- bucket_name, object_name, file_path, content_type
- metadata
- sse, progress, part_size
- num_parallel_uploads
- tags, retention, legal_hold
- (keyword-only separator)
- headers, user_metadata, checksum
- region, extra_headers, extra_query_params
Currently, headers, user_metadata, and checksum are listed before sse and other positional parameters, which doesn't match the signature where they appear after the *, separator.
__Parameters__
| Param | Type | Description |
|:-----------------------|:------------------------------------------------|:-------------------------------------------|
| `bucket_name` | _str_ | Name of the bucket. |
| `object_name` | _str_ | Object name in the bucket. |
| `file_path` | _str_ | Name of file to upload. |
| `content_type` | _str = "application/octet-stream"_ | Content type of the object. |
| `metadata` | _Optional[minio.helpers.HTTPHeaderDict] = None_ | Metadata for the object (legacy). |
| `headers` | _Optional[minio.helpers.HTTPHeaderDict] = None_ | Additional headers. |
| `user_metadata` | _Optional[minio.helpers.HTTPHeaderDict] = None_ | User metadata of the object. |
| `sse` | _Optional[minio.sse.Sse] = None_ | Server-side encryption. |
| `progress` | _Optional[minio.helpers.ProgressType] = None_ | A progress object. |
| `part_size` | _int = 0_ | Multipart part size. |
| `checksum` | _Optional[minio.checksum.Algorithm] = None_ | Algorithm for checksum computation. |
| `num_parallel_uploads` | _int = 3_ | Number of parallel uploads. |
| `tags` | _Optional[minio.commonconfig.Tags] = None_ | Tags for the object. |
| `retention` | _Optional[minio.retention.Retention] = None_ | Retention configuration. |
| `legal_hold` | _bool = False_ | Flag to set legal hold for the object. |
| `region` | _Optional[str] = None_ | Region of the bucket to skip auto probing. |
| `extra_headers` | _Optional[minio.helpers.HTTPHeaderDict] = None_ | Extra headers for advanced usage. |
| `extra_query_params` | _Optional[minio.helpers.HTTPQueryDict] = None_ | Extra query parameters for advanced usage. |
docs/API.md:2201
- The function signature and parameter table in the documentation don't match the implementation. The documentation is missing the new
response_headersparameter and doesn't reflect thatregionis now a keyword-only argument.
The signature should be:
get_presigned_url(self, method: str, bucket_name: str, object_name: str, expires: timedelta = timedelta(days=7), response_headers: Optional[HTTPHeaderDict] = None, request_date: Optional[datetime] = None, version_id: Optional[str] = None, extra_query_params: Optional[HTTPQueryDict] = None, *, region: Optional[str] = None)
And the parameter table should include an entry for response_headers between expires and request_date.
### get_presigned_url(self, method: str, bucket_name: str, object_name: str, expires: timedelta = timedelta(days=7), request_date: Optional[datetime] = None, version_id: Optional[str] = None, region: Optional[str] = None, extra_query_params: Optional[HTTPQueryDict] = None) -> str
Get presigned URL of an object for HTTP method, expiry time and custom request parameters.
__Parameters__
| Param | Type | Description |
|:---------------------|:--------------------------------------------------|:-------------------------------------------|
| `method` | _str_ | HTTP method. |
| `bucket_name` | _str_ | Name of the bucket. |
| `object_name` | _str_ | Object name in the bucket. |
| `expires` | _datetime.timedelta = datetime.timedelta(days=7)_ | Expiry in seconds. |
| `request_date` | _Optional[datetime.datetime] = None_ | Request time instead of current time. |
| `version_id` | _Optional[str] = None_ | Version ID of the object. |
| `region` | _Optional[str] = None_ | Region of the bucket to skip auto probing. |
| `extra_query_params` | _Optional[minio.helpers.HTTPQueryDict] = None_ | Extra query parameters for advanced usage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not prefer this kind of fix and I recommend full backward compatibility. I am not sure how much this PR does. My strong recommendations are
We broke backward compatibility, so this PR reverts to restore the expected functionality. Not sure what you are saying. Once this PR is merged, we will release v7.2.20, so users who were on v7.2.18 won't see anything break when they upgrade. |
|
Trying a new PR as well, yes, then we can pick. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
docs/API.md:2201
- The parameter table for
get_presigned_urlis missing theresponse_headersparameter. According to the implementation,response_headersshould be documented betweenexpiresandrequest_datewith typeOptional[minio.helpers.HTTPHeaderDict] = Noneand description for response headers in advanced usage. Additionally,regionshould be marked as keyword-only to match the implementation.
| `expires` | _datetime.timedelta = datetime.timedelta(days=7)_ | Expiry in seconds. |
| `request_date` | _Optional[datetime.datetime] = None_ | Request time instead of current time. |
| `version_id` | _Optional[str] = None_ | Version ID of the object. |
| `region` | _Optional[str] = None_ | Region of the bucket to skip auto probing. |
| `extra_query_params` | _Optional[minio.helpers.HTTPQueryDict] = None_ | Extra query parameters for advanced usage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Fix breaking changes from v7.2.19
Problem
Commit 6daf366 introduced keyword-only argument separators (
*,) at the start of function signatures, breaking backward compatibility for all existing client code using positional arguments.Solution
backward compatibility
region,extra_headers,extra_query_params) to enforce keyword-only usage for newadditions
metadataparameter AND newheaders/user_metadataparameters withfallback logic for backward compatibility
Changes
parameter to
compose_objectwith correct parameter ordering and keyword-only separators
max-positional-arguments=16to accommodatebackward-compatible signatures
Backward Compatibility
✅ Old code continues to work:
client.put_object(bucket, obj, data, length)✅ New parameters require keywords:
client.put_object(..., region="us-east-1")✅ Legacy metadata parameter supported:
metadata={"key": "val"}automatically falls back touser_metadata