Fix open S3Object InputStream #71
Conversation
…make it reusable and prevent a wrong usage because of an unused open InputStream. Signed-off-by: Michael Herdt <Michael.Herdt@bosch.io>
Signed-off-by: Michael Herdt <Michael.Herdt@bosch.io>
Signed-off-by: Michael Herdt <Michael.Herdt@bosch.io>
Signed-off-by: Michael Herdt <Michael.Herdt@bosch.io>
Signed-off-by: Michael Herdt <Michael.Herdt@bosch.io>
…on before closing it. Signed-off-by: Michael Herdt <Michael.Herdt@bosch.io>
StefanKlt
left a comment
StefanKlt
left a comment
There was a problem hiding this comment.
Looks good, thanks @herdt-michael 👍
|
@hawkbit-bot verify please |
|
|
||
| private static DbArtifactHash createArtifactHash(final String artifactId, ObjectMetadata metadata) { | ||
| return new DbArtifactHash(artifactId, BaseEncoding.base16().lowerCase() | ||
| .encode(BaseEncoding.base64().decode(sanitizeEtag(metadata.getETag()))), null); |
| } | ||
|
|
||
| private static S3Object getS3ObjectOrThrowException(AmazonS3 amazonS3, String bucketName, String key) | ||
| throws S3ArtifactNotFoundException { |
There was a problem hiding this comment.
| * in case that no artifact could be found for the given values | ||
| */ | ||
| public static S3Artifact get(final AmazonS3 amazonS3, final S3RepositoryProperties s3Properties, final String key, | ||
| final String artifactId) throws S3ArtifactNotFoundException { |
There was a problem hiding this comment.
|
SonarQube analysis reported 3 issues
Watch the comments in this conversation to review them. |
Signed-off-by: Michael Herdt <Michael.Herdt@bosch.io>
|
|
||
| private void refreshS3ObjectIfNeeded() { | ||
| if (s3Object == null || s3InputStream == null) { | ||
| LOG.info("Initialize S3Object in bucket {} with key {}", s3Properties.getBucketName(), key); |
There was a problem hiding this comment.
I guess we could change the log level to debug otherwise we would get a lot of log messages for each file download
| private void refreshS3ObjectIfNeeded() { | ||
| if (s3Object == null || s3InputStream == null) { | ||
| LOG.info("Initialize S3Object in bucket {} with key {}", s3Properties.getBucketName(), key); | ||
| s3Object = amazonS3.getObject(s3Properties.getBucketName(), key); |
There was a problem hiding this comment.
why isn't the getS3ObjectOrThrowException method used here? s3Object can be potentially null
| try { | ||
| return S3Artifact.get(amazonS3, s3Properties, key, sha1Hash); | ||
| } catch (final S3ArtifactNotFoundException e) { | ||
| LOG.debug("Cannot find artifact for bucket {} with key {}", e.getBucket(), e.getKey(), e); |
There was a problem hiding this comment.
here, however I would use level "warn" because we always execute existsByTenantAndSha1 beforehand in usage scenarios
| } | ||
|
|
||
| private static S3Object getS3ObjectOrThrowException(AmazonS3 amazonS3, String bucketName, String key) { | ||
| final S3Object s3Object = amazonS3.getObject(bucketName, key); |
There was a problem hiding this comment.
we should also catch SdkClientException as well as AmazonServiceException here
|
Frankly speaking I believe that proposed changes make the code less understandable with S3Artifact being mutable and used in different context (e.g. |
4 participants
Open S3Object InputStream when creating an instance of S3Artifact to make it reusable and prevent a wrong usage because of an unused open InputStream.
Signed-off-by: Michael Herdt Michael.Herdt@bosch.io