Introduce a ResolvedBucketName type to avoid temporal coupling (and e… #1754
Conversation
…nhance readability)
| } | ||
|
|
||
| private boolean isNameSpace(BucketName bucketName) { | ||
| return namespace | ||
| .map(existingNamespace -> existingNamespace.equals(bucketName)) | ||
| .map(existingNamespace -> existingNamespace.asString().equals(bucketName.asString())) |
There was a problem hiding this comment.
💭 thought: the namespace term is also called defaultBucketName in other places ( it is called namespace in the configuration file, the configuration builder accepts a namespace setter but when building the actual configuration object it injects the namespace into a field named defaultBucketName a similarly named field is also exposed by the S3 dao but in the bucketname resolver it is again called namespace. It is unclear to me what this concept is supposed to represent and how it is different from bucketPrefix (whose name is fairly stable throughout the code)
I don't expect an action on this MR but seeing the name here reminded me of this thought.
There was a problem hiding this comment.
❔ question: why change the equality from BucketName#equals to asString().equals I agree that the result is identical but don't think this change makes the code clearer
I understand why it is necessary in the overload of isNameSpace that compares a ResolvedBucketName and a BucketName.
server/blob/blob-s3/src/main/java/org/apache/james/blob/objectstorage/aws/S3BlobStoreDAO.java
Show resolved
Hide resolved
server/blob/blob-s3/src/main/java/org/apache/james/blob/objectstorage/aws/S3BlobStoreDAO.java
Show resolved
Hide resolved
|
|
||
| import java.util.Objects; | ||
|
|
||
| public final class ResolvedBucketName { |
There was a problem hiding this comment.
This POJO is defined in the API but only used in blob-s3.
I think introducing it is a good idea, but we should avoid poluting the API and move it into blob-s3.
There was a problem hiding this comment.
Hi @chibenwa ,
Do you have a plan to migrate to Java 14 (or 16), this will prevent from lot of code like this one.
There was a problem hiding this comment.
If we upgrade better a LTS version, so 17 (or even 21 would be better, but maybe too big of a jump?)
I would support such a migration, but I'm not sure the all community is ready to upgrade their java version :)
Could perhaps start a thread on the ML though?
There was a problem hiding this comment.
Hi @chibenwa ,
Do you have a plan to migrate to Java 14 (or 16), this will prevent from lot of code like this one.
You is out of place, as a community we should embrasse a we.
I would personnalyy support a switch to Java 17 or even 21.
Do you think you can pull off a mail on this topic on the mailing lists?
|
|
@mbaechler status on this? any plan on fixing soon checkstyle issues and answering comments? :) |
I apologize for dropping PRs without a proper follow-up plan. |
… (and enhance readability)
|
https://ci-builds.apache.org/job/james/job/ApacheJames/job/PR-1754/4/testReport/ I think the errors in BucketNameResolverTest are related |
…nhance readability)