Copilot commented on code in PR #9815:
URL: https://github.com/apache/ozone/pull/9815#discussion_r3012836701
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -1127,7 +1148,48 @@ private CopyObjectResponse copyObject(OzoneVolume volume,
}
}
}
-
+
+ /**
+ * Opens a key for put, applying conditional write logic based on
+ * If-None-Match and If-Match headers.
+ */
+ @SuppressWarnings("checkstyle:ParameterNumber")
+ private OzoneOutputStream openKeyForPut(String volumeName, String bucketName,
+ OzoneBucket bucket, String keyPath, long length,
+ ReplicationConfig replicationConfig, Map<String, String> customMetadata,
+ Map<String, String> tags, String ifNoneMatch, String ifMatch)
+ throws IOException {
+ if (ifNoneMatch != null && "*".equals(ifNoneMatch.trim())) {
+ return getClientProtocol().createKeyIfNotExists(
+ volumeName, bucketName, keyPath, length, replicationConfig,
+ customMetadata, tags);
Review Comment:
If-None-Match handling only recognizes an unquoted `*` and otherwise
silently falls back to an unconditional createKey(). Per the design doc,
clients may send `If-None-Match: "*"`; in that case this code would incorrectly
allow overwrites. Consider normalizing/parsing the header value (similar to
parseETag) and rejecting unsupported If-None-Match values instead of ignoring
them.
```suggestion
if (ifNoneMatch != null) {
// Normalize the If-None-Match header value, accepting both quoted and
// unquoted "*" and rejecting any other value instead of silently
// falling back to an unconditional createKey().
String normalizedIfNoneMatch = parseETag(ifNoneMatch);
if ("*".equals(normalizedIfNoneMatch)) {
return getClientProtocol().createKeyIfNotExists(
volumeName, bucketName, keyPath, length, replicationConfig,
customMetadata, tags);
} else {
throw new OS3Exception(
PRECOND_FAILED,
"Unsupported If-None-Match header value",
HttpStatus.SC_PRECONDITION_FAILED);
}
```
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -1127,7 +1148,48 @@ private CopyObjectResponse copyObject(OzoneVolume volume,
}
}
}
-
+
+ /**
+ * Opens a key for put, applying conditional write logic based on
+ * If-None-Match and If-Match headers.
+ */
+ @SuppressWarnings("checkstyle:ParameterNumber")
+ private OzoneOutputStream openKeyForPut(String volumeName, String bucketName,
+ OzoneBucket bucket, String keyPath, long length,
Review Comment:
The openKeyForPut(...) helper takes an OzoneBucket parameter but does not
use it. Removing the unused parameter will simplify the signature and avoid
confusion about whether bucket-specific info is required for conditional puts.
```suggestion
String keyPath, long length,
```
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -1127,7 +1148,48 @@ private CopyObjectResponse copyObject(OzoneVolume volume,
}
}
}
-
+
+ /**
+ * Opens a key for put, applying conditional write logic based on
+ * If-None-Match and If-Match headers.
+ */
+ @SuppressWarnings("checkstyle:ParameterNumber")
+ private OzoneOutputStream openKeyForPut(String volumeName, String bucketName,
+ OzoneBucket bucket, String keyPath, long length,
+ ReplicationConfig replicationConfig, Map<String, String> customMetadata,
+ Map<String, String> tags, String ifNoneMatch, String ifMatch)
+ throws IOException {
+ if (ifNoneMatch != null && "*".equals(ifNoneMatch.trim())) {
+ return getClientProtocol().createKeyIfNotExists(
+ volumeName, bucketName, keyPath, length, replicationConfig,
+ customMetadata, tags);
+ } else if (ifMatch != null) {
+ String expectedETag = parseETag(ifMatch);
+ return getClientProtocol().rewriteKeyIfMatch(
+ volumeName, bucketName, keyPath, length, expectedETag,
+ replicationConfig, customMetadata, tags);
+ } else {
+ return getClientProtocol().createKey(
+ volumeName, bucketName, keyPath, length, replicationConfig,
+ customMetadata, tags);
+ }
+ }
+
+ /**
+ * Parses an ETag from a conditional header value, removing surrounding
+ * quotes if present.
+ */
+ static String parseETag(String headerValue) {
+ if (headerValue == null) {
+ return null;
+ }
+ String etag = headerValue.trim();
+ if (etag.startsWith("\"") && etag.endsWith("\"")) {
+ return etag.substring(1, etag.length() - 1);
+ }
+ return etag;
Review Comment:
parseETag duplicates quote-stripping logic that already exists as
S3Utils.stripQuotes (and is statically imported in this class). Consider
reusing stripQuotes(headerValue.trim()) to keep ETag normalization consistent
across the endpoint and reduce duplicate parsing behavior.
```suggestion
return stripQuotes(headerValue.trim());
```
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -1127,7 +1148,48 @@ private CopyObjectResponse copyObject(OzoneVolume volume,
}
}
}
-
+
+ /**
+ * Opens a key for put, applying conditional write logic based on
+ * If-None-Match and If-Match headers.
+ */
+ @SuppressWarnings("checkstyle:ParameterNumber")
+ private OzoneOutputStream openKeyForPut(String volumeName, String bucketName,
+ OzoneBucket bucket, String keyPath, long length,
+ ReplicationConfig replicationConfig, Map<String, String> customMetadata,
+ Map<String, String> tags, String ifNoneMatch, String ifMatch)
+ throws IOException {
+ if (ifNoneMatch != null && "*".equals(ifNoneMatch.trim())) {
+ return getClientProtocol().createKeyIfNotExists(
+ volumeName, bucketName, keyPath, length, replicationConfig,
+ customMetadata, tags);
+ } else if (ifMatch != null) {
+ String expectedETag = parseETag(ifMatch);
+ return getClientProtocol().rewriteKeyIfMatch(
+ volumeName, bucketName, keyPath, length, expectedETag,
+ replicationConfig, customMetadata, tags);
+ } else {
Review Comment:
If-Match supports the standard `*` form in HTTP to mean "any current
representation". Right now `If-Match: *` will be treated as an expected ETag of
`*` and will always fail unless the stored ETag is literally `*`. Consider
explicitly handling `*` (or returning INVALID_REQUEST if you intentionally
don't support it) so behavior is not surprising for clients.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
