Copilot commented on code in PR #1246:
URL: https://github.com/apache/pekko-connectors/pull/1246#discussion_r2562996589
##########
s3/src/main/scala/org/apache/pekko/stream/connectors/s3/S3Headers.scala:
##########
@@ -77,8 +77,10 @@ final class S3Headers private (val cannedAcl:
Option[CannedAcl] = None,
RawHeader(header._1, header._2)
}
- @InternalApi private[s3] def headersFor(request: S3Request) =
- headers ++ serverSideEncryption.toIndexedSeq.flatMap(_.headersFor(request))
+ @InternalApi private[s3] def headersFor(request: S3Request)(implicit
s3Settings: S3Settings) =
+ headers.filter(header =>
+ s3Settings.concreateAllowedHeaders.getOrElse(request,
Set.empty).contains(
Review Comment:
Typo in variable name: `concreateAllowedHeaders` should be
`concreteAllowedHeaders` (misspelling of 'concrete'). This should be corrected
to match the fix in settings.scala.
```suggestion
s3Settings.concreteAllowedHeaders.getOrElse(request,
Set.empty).contains(
```
##########
s3/src/main/scala/org/apache/pekko/stream/connectors/s3/settings.scala:
##########
@@ -471,6 +493,7 @@ final class S3Settings private (
Objects.equals(this.retrySettings, that.retrySettings) &&
Objects.equals(this.multipartUploadSettings, multipartUploadSettings) &&
this.signAnonymousRequests == that.signAnonymousRequests
Review Comment:
Missing logical AND operator (&&) before `this.allowedHeaders ==
that.allowedHeaders`. This will cause a compilation error as the expression is
not properly connected to the previous condition.
```suggestion
this.signAnonymousRequests == that.signAnonymousRequests &&
```
##########
s3/src/test/scala/docs/scaladsl/S3SinkSpec.scala:
##########
@@ -240,6 +240,8 @@ class S3SinkSpec extends S3WireMockBase with
S3ClientIntegrationSpec with Option
val requestPayerHeader = "x-amz-request-payer"
val requestPayerHeaderValue = "requester"
+ val storgeClassHeader = "x-amz-storage-class"
Review Comment:
Typo in variable name: `storgeClassHeader` should be `storageClassHeader`
(missing 'a'). This inconsistency could lead to confusion since the value
variable is correctly named `storageClassHeaderValue`.
##########
s3/src/test/scala/docs/scaladsl/S3SinkSpec.scala:
##########
@@ -286,7 +294,9 @@ class S3SinkSpec extends S3WireMockBase with
S3ClientIntegrationSpec with Option
.withHeader(sseCKeyHeader, new EqualToPattern(sseCKeyHeaderValue))
.withHeader(sseCSourceAlgorithmHeader, new
EqualToPattern(sseCSourceAlgorithmHeaderValue))
.withHeader(sseCSourceKeyHeader, new
EqualToPattern(sseCSourceKeyHeaderValue))
- .withHeader(requestPayerHeader, new
EqualToPattern(requestPayerHeaderValue)))
+ .withHeader(requestPayerHeader, new
EqualToPattern(requestPayerHeaderValue))
+ .withoutHeader(storgeClassHeader)
Review Comment:
Typo in variable name: `storgeClassHeader` should be `storageClassHeader`
(missing 'a').
##########
s3/src/main/scala/org/apache/pekko/stream/connectors/s3/settings.scala:
##########
@@ -420,6 +427,17 @@ final class S3Settings private (
def withSignAnonymousRequests(value: Boolean): S3Settings =
if (signAnonymousRequests == value) this else copy(signAnonymousRequests =
value)
+ private[s3] val concreateAllowedHeaders: Map[S3Request, Set[String]] = {
Review Comment:
Typo in variable name: `concreateAllowedHeaders` should be
`concreteAllowedHeaders` (misspelling of 'concrete'). This naming error could
cause confusion when reading the code.
```suggestion
private[s3] val concreteAllowedHeaders: Map[S3Request, Set[String]] = {
```
##########
s3/src/main/scala/org/apache/pekko/stream/connectors/s3/settings.scala:
##########
@@ -442,7 +462,9 @@ final class S3Settings private (
validateObjectKey,
retrySettings,
multipartUploadSettings,
- signAnonymousRequests)
+ signAnonymousRequests,
+ allowedHeaders
+ )
override def toString: String =
"S3Settings(" +
Review Comment:
The `toString()` method is missing the `allowedHeaders` field. For
consistency and completeness in debugging, the string representation should
include all fields of the object, especially newly added ones like
`allowedHeaders`.
##########
s3/src/test/scala/docs/scaladsl/S3SinkSpec.scala:
##########
@@ -263,7 +265,13 @@ class S3SinkSpec extends S3WireMockBase with
S3ClientIntegrationSpec with Option
targetBucketKey,
s3Headers = S3Headers()
.withServerSideEncryption(keys)
- .withCustomHeaders(Map(requestPayerHeader ->
requestPayerHeaderValue)))
+ .withCustomHeaders(
+ Map(
+ requestPayerHeader -> requestPayerHeaderValue,
+ storgeClassHeader -> storageClassHeaderValue
Review Comment:
Typo in variable name: `storgeClassHeader` should be `storageClassHeader`
(missing 'a').
##########
s3/src/main/scala/org/apache/pekko/stream/connectors/s3/settings.scala:
##########
@@ -420,6 +427,17 @@ final class S3Settings private (
def withSignAnonymousRequests(value: Boolean): S3Settings =
if (signAnonymousRequests == value) this else copy(signAnonymousRequests =
value)
+ private[s3] val concreateAllowedHeaders: Map[S3Request, Set[String]] = {
+ allowedHeaders.foldLeft(Map.empty[S3Request, Set[String]]) {
+ case (acc, (header, value)) =>
+ S3Request.fromString(header) match {
+ case OptionVal.Some(header) => acc + (header -> value)
+ case OptionVal.None => acc
+ case other => throw new MatchError(other)
Review Comment:
The `MatchError` case at line 436 is unreachable. `OptionVal` only has two
possible values: `OptionVal.Some` and `OptionVal.None`, both of which are
already handled in the preceding cases. This default case can never be executed
and should be removed to avoid confusion.
```suggestion
```
--
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]
