- fix bug: change sql-query in localArtifactRepository #820

nazifebasbaz · 2019-04-09T13:13:46Z

tests added in ArtifactManagementTest:
-ensure that an Artifact can not deleted when the Artifact with the
same hash exist
-ensure that Artifacts with the same hash from one tenant can not
deleted by an other tenant

Signed-off-by: Nazife Basbaz [email protected]

hawkbit · 2019-04-09T13:13:51Z

Thanks for taking the time to contribute to hawkBit! We really appreciate this. Make yourself comfortable while I'm looking for a committer to help you with your contribution.
Please make sure you read the contribution guide and signed the Eclipse Contributor Agreement (ECA).

nazifebasbaz · 2019-04-25T11:32:41Z

...-repository-jpa/src/test/java/org/eclipse/hawkbit/repository/jpa/ArtifactManagementTest.java

+
+        createArtifactForTenant(tenant1, "myInput", module.getId(), "myFirstFile");
+        createArtifactForTenant(tenant2, "myInput", module2.getId(), "mySecondFile");
+        final Artifact artifactTenant2 = createArtifactForTenant(tenant2, "myInput", module2.getId(), "myThirdFile");


already tested with the previous test case

nazifebasbaz · 2019-04-25T11:37:40Z

check other artifact storage extensions if file handling is covered as well

a-sayyed · 2019-05-14T13:28:47Z

...repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/LocalArtifactRepository.java

     */
-    @Query("SELECT CASE WHEN COUNT(a)>0 THEN 'true' ELSE 'false' END FROM JpaArtifact a WHERE a.sha1Hash = :sha1 AND a.softwareModule.id != :moduleId AND a.softwareModule.deleted = 0")
-    boolean existsWithSha1HashAndSoftwareModuleIdIsNot(@Param("sha1") String sha1, @Param("moduleId") Long moduleId);
+    @Query("SELECT CASE WHEN COUNT (a)>1 THEN 'true' ELSE 'false' END FROM JpaArtifact a WHERE a.sha1Hash = :sha1 AND a.tenant = :tenant AND a.softwareModule.deleted = 0")


we can get rid of the @Query annotation and use the Jpa support for Entities:

long countBySha1HashAndTenantAndSoftwareModuleDeletedIsFalse(@Param("sha1") String sha1, @Param("tenant") String tenant);

We can later just check the count if greater than 1

a-sayyed · 2019-05-14T13:33:13Z

...repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/LocalArtifactRepository.java

-     * Verifies if an artifact exists that has given hash and is still related
-     * to a {@link SoftwareModule} other than a given one and not
-     * {@link SoftwareModule#isDeleted()}.
+     * Verifies if in the same tenant one then more {@link Artifact}s with the


Needs to be adapted:

/** * Counts current elements based on the sha1 and tenant, as well as having * the {@link SoftwareModule} property 'deleted' with value 'false' * * @param sha1 * the sha1 of the {@link Artifact} * @param tenant * the current tenant * * @return the count of the elements */

a-sayyed · 2019-05-14T13:41:49Z

...t-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/JpaArtifactManagement.java

@@ -166,8 +165,8 @@ private void assertMaxArtifactStorageQuota(final String filename, final long art
    @Retryable(include = {
            ConcurrencyFailureException.class }, maxAttempts = Constants.TX_RT_MAX, backoff = @Backoff(delay = Constants.TX_RT_DELAY))
    public boolean clearArtifactBinary(final String sha1Hash, final long moduleId) {
-
-        if (localArtifactRepository.existsWithSha1HashAndSoftwareModuleIdIsNot(sha1Hash, moduleId)) {
+        if (localArtifactRepository.existsForMoreThenOneArtifactInTheSameTenantWithSha1HashAndSoftwareModuleIsNotDeleted(sha1Hash,


Could be refactored:

final long count = localArtifactRepository.countBySha1HashAndTenantAndSoftwareModuleDeletedIsFalse(sha1Hash, tenantAware.getCurrentTenant()); if (count > 1) { // there are still other artifacts that need the binary return false; }

a-sayyed · 2019-05-14T13:55:56Z

...-repository-jpa/src/test/java/org/eclipse/hawkbit/repository/jpa/ArtifactManagementTest.java

+    @Test
+    @Description("Verifies that you cannot delete an artifact which exists with the same hash, in the same tenant and the SoftwareModule is not deleted .")
+    public void deleteArtifactWithSameHashAndSoftwareModuleIsNotDeletedInSameTenants()
+            throws NoSuchAlgorithmException, IOException {


remove the throws NoSuchAlgorithmException, not thrown in this context.
Could you please also double check in the whole file, there are many redundant throws declarations like this (from before) that need to be cleaned up.

a-sayyed · 2019-05-14T13:59:56Z

...-repository-jpa/src/test/java/org/eclipse/hawkbit/repository/jpa/ArtifactManagementTest.java

+                    .existsForMoreThenOneArtifactInTheSameTenantWithSha1HashAndSoftwareModuleIsNotDeleted(
+                            artifact1.getSha1Hash(), artifact1.getTenant())).isTrue();
+
+            artifactRepository.deleteById(artifact1.getId());


maybe here we can add another assertion for the binary file contents

try (final InputStream inputStream = artifactManagement.loadArtifactBinary(artifact2.getSha1Hash()).get() .getFileInputStream()) { assertTrue("The stored binary matches the given binary", IOUtils.contentEquals(new ByteArrayInputStream(randomBytes), inputStream)); }

Btw, this assertion is used in other places in this class, so maybe you can extract it to an own method, maybe with the name assertEqualFileContents(Optional<AbstractDBArtifact> artifact, byte[] randomBytes)

StefanKlt · 2019-05-16T13:09:30Z

...repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/LocalArtifactRepository.java

     */
-    @Query("SELECT CASE WHEN COUNT(a)>0 THEN 'true' ELSE 'false' END FROM JpaArtifact a WHERE a.sha1Hash = :sha1 AND a.softwareModule.id != :moduleId AND a.softwareModule.deleted = 0")
-    boolean existsWithSha1HashAndSoftwareModuleIdIsNot(@Param("sha1") String sha1, @Param("moduleId") Long moduleId);
+    default boolean existsForMoreThenOneArtifactInTheSameTenantWithSha1HashAndSoftwareModuleIsNotDeleted(


is this method used anywhere or can it be removed?

good catch!

hawkbit-bot · 2019-05-16T15:21:28Z

hawkbit-core/src/main/java/org/eclipse/hawkbit/artifact/repository/ArtifactRepository.java

+     *
+     * @return the boolean whether the atrifact exists or not
+     */
+    boolean existsByTenantAndSha1(@NotEmpty String Tenant, @NotEmpty String sha1Hash);


Document the parameter(s): Tenant
Rename this local variable to match the regular expression '^[a-z][a-zA-Z0-9]*$'.

hawkbit-bot · 2019-05-16T15:21:30Z

SonarQube analysis reported 2 issues

2 minor

Watch the comments in this conversation to review them.

bogdan-bondar · 2019-05-22T13:24:18Z

...t-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/JpaArtifactManagement.java


        assertArtifactQuota(moduleId, 1);
        assertMaxArtifactSizeQuota(filename, moduleId, artifactUpload.getFilesize());
        assertMaxArtifactStorageQuota(filename, artifactUpload.getFilesize());

+        AbstractDbArtifact result = getOrCreateArtifact(artifactUpload);


You could use the Optional in order to get rid of null checks:

Suggested change

AbstractDbArtifact result = getOrCreateArtifact(artifactUpload);

return getOrCreateArtifact(artifactUpload)

.map(artifact -> storeArtifactMetadata(softwareModule, filename, artifact, existing)).orElse(null);

Yes, looks much cleaner indeed, thank you for the hint! 👍

bogdan-bondar · 2019-05-22T13:25:15Z

...t-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/JpaArtifactManagement.java

+        return result == null ? null : storeArtifactMetadata(softwareModule, filename, result, existing);
+    }
+
+    private AbstractDbArtifact getOrCreateArtifact(final ArtifactUpload artifactUpload) {


Then you need to change this method correspondingly:

Suggested change

private AbstractDbArtifact getOrCreateArtifact(final ArtifactUpload artifactUpload) {

private Optional<AbstractDbArtifact> getOrCreateArtifact(final ArtifactUpload artifactUpload) {

final String providedSha1Sum = artifactUpload.getProvidedSha1Sum();

if (!StringUtils.isEmpty(providedSha1Sum)) {

final AbstractDbArtifact artifact = artifactRepository.getArtifactBySha1(tenantAware.getCurrentTenant(),

providedSha1Sum);

if (artifact != null) {

return Optional.of(artifact);

}

}

return Optional.ofNullable(storeArtifact(artifactUpload));

}

bogdan-bondar · 2019-05-22T17:42:56Z

...t-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/JpaArtifactManagement.java

+        if (!StringUtils.isEmpty(providedSha1Sum)) {
+            artifact = artifactRepository.getArtifactBySha1(tenantAware.getCurrentTenant(), providedSha1Sum);
+        }
+        return Optional.ofNullable(artifact == null ? storeArtifact(artifactUpload) : artifact);


I would still propose my previous solution (with 2 return statements and final AbstractDbArtifact) in order to get rid of conditional here

Yes, but then we have "multiple return statements" and we still have to do the null check anyway.
I could move the check to an own line to make it simpler maybe. What do you think?

artifact = (artifact == null) ? storeArtifact(artifactUpload) : artifact; return Optional.ofNullable(artifact);

Ok, let it be our consensus for this question :)

bogdan-bondar

Looks Good!

- tests added in ArtifactManagementTest: -ensure that an Artifact can not deleted when the Artifact with the same hash exist -ensure that Artifacts with the same hash from one tenant can not deleted by an other tenant Signed-off-by: Nazife Basbaz <[email protected]>

Signed-off-by: Nazife Basbaz<[email protected]>

Signed-off-by: Nazife Basbaz <[email protected]>

@query

-replace the @query annotation with the Jpa support for entities -adapt tests -remove unused exceptions Signed-off-by: Nazife Basbaz <[email protected]>

Signed-off-by: Ahmed Sayed <[email protected]>

Signed-off-by: Nazife Basbaz <[email protected]>

Signed-off-by: Ahmed Sayed <[email protected]>

…#820) Also-by: Nazife Basbaz <[email protected]> Signed-off-by: Ahmed Sayed <[email protected]>

schabdo added this to the 0.3.0M3 milestone Apr 9, 2019

nazifebasbaz commented Apr 25, 2019

View reviewed changes

nazifebasbaz force-pushed the bugfix_uploadingIdenticalArtifacts branch from 20199f2 to 2b738c3 Compare May 14, 2019 12:32

a-sayyed reviewed May 14, 2019

View reviewed changes

a-sayyed mentioned this pull request May 16, 2019

added artifact repository implementation for existsByTenantAndSha1() eclipse-hawkbit/hawkbit-extensions#45

Merged

StefanKlt reviewed May 16, 2019

View reviewed changes

hawkbit-bot reviewed May 16, 2019

View reviewed changes

nazifebasbaz force-pushed the bugfix_uploadingIdenticalArtifacts branch from 6b15408 to 3080740 Compare May 17, 2019 12:59

schabdo modified the milestones: 0.3.0M3, 0.3.0M4 May 21, 2019

bogdan-bondar reviewed May 22, 2019

View reviewed changes

a-sayyed force-pushed the bugfix_uploadingIdenticalArtifacts branch from 39a0126 to 06a198f Compare May 23, 2019 14:28

bogdan-bondar approved these changes May 23, 2019

View reviewed changes

nazifebasbaz and others added 8 commits May 27, 2019 12:36

-delete blanks and add parameter

2afb647

Signed-off-by: Nazife Basbaz<[email protected]>

delete already tested test case

e7d3de2

Signed-off-by: Nazife Basbaz <[email protected]>

fix pull request

265b532

-replace the @query annotation with the Jpa support for entities -adapt tests -remove unused exceptions Signed-off-by: Nazife Basbaz <[email protected]>

added assertion for duplicate files

eec384e

Signed-off-by: Ahmed Sayed <[email protected]>

added assertion for duplicate files with the same sha1 hash

c757c11

Signed-off-by: Ahmed Sayed <[email protected]>

removed unused method

d7ebbf1

Signed-off-by: Ahmed Sayed <[email protected]>

fixed typo in method parameter name

9849b57

Signed-off-by: Ahmed Sayed <[email protected]>

a-sayyed and others added 8 commits May 27, 2019 12:36

Code cleanup

dc6aa0f

Signed-off-by: Ahmed Sayed <[email protected]>

removed unnecessary semicolon

2f73b40

Signed-off-by: Nazife Basbaz <[email protected]>

removed unnecessary check for file exists before calling getFile

f3e286f

Signed-off-by: Ahmed Sayed <[email protected]>

Use Optional instead of null checks

dff0f6f

Signed-off-by: Ahmed Sayed <[email protected]>

simplified expression

ed72f08

Signed-off-by: Ahmed Sayed <[email protected]>

removed "always true" expressions

da7d81c

Signed-off-by: Ahmed Sayed <[email protected]>

inlined variable

f1bc861

Signed-off-by: Ahmed Sayed <[email protected]>

simplified method

6f0a0fc

Signed-off-by: Ahmed Sayed <[email protected]>

a-sayyed force-pushed the bugfix_uploadingIdenticalArtifacts branch from 06a198f to 6f0a0fc Compare May 27, 2019 10:43

laverman merged commit fde0cbd into eclipse-hawkbit:master Jun 5, 2019

laverman deleted the bugfix_uploadingIdenticalArtifacts branch June 5, 2019 14:07

AmmarBikic pushed a commit to bosch-io/hawkbit that referenced this pull request Oct 2, 2020

fix bug: change sql-query in localArtifactRepository (eclipse-hawkbit…

3dcbe0f

…#820) Also-by: Nazife Basbaz <[email protected]> Signed-off-by: Ahmed Sayed <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

- fix bug: change sql-query in localArtifactRepository #820

- fix bug: change sql-query in localArtifactRepository #820

nazifebasbaz commented Apr 9, 2019

hawkbit bot commented Apr 9, 2019

nazifebasbaz Apr 25, 2019

nazifebasbaz commented Apr 25, 2019

a-sayyed May 14, 2019 •

edited

Loading

a-sayyed May 14, 2019 •

edited

Loading

a-sayyed May 14, 2019

a-sayyed May 14, 2019 •

edited

Loading

a-sayyed May 14, 2019 •

edited

Loading

a-sayyed May 20, 2019

StefanKlt May 16, 2019

a-sayyed May 16, 2019

hawkbit-bot May 16, 2019

hawkbit-bot commented May 16, 2019

bogdan-bondar May 22, 2019

a-sayyed May 22, 2019

bogdan-bondar May 22, 2019

bogdan-bondar May 22, 2019

a-sayyed May 23, 2019

bogdan-bondar May 23, 2019

bogdan-bondar left a comment

	AbstractDbArtifact result = getOrCreateArtifact(artifactUpload);
	return getOrCreateArtifact(artifactUpload)
	.map(artifact -> storeArtifactMetadata(softwareModule, filename, artifact, existing)).orElse(null);

-    private AbstractDbArtifact getOrCreateArtifact(final ArtifactUpload artifactUpload) {
+    private Optional<AbstractDbArtifact> getOrCreateArtifact(final ArtifactUpload artifactUpload) {
+        final String providedSha1Sum = artifactUpload.getProvidedSha1Sum();
+        if (!StringUtils.isEmpty(providedSha1Sum)) {
+            final AbstractDbArtifact artifact = artifactRepository.getArtifactBySha1(tenantAware.getCurrentTenant(),
+                    providedSha1Sum);
+            if (artifact != null) {
+                return Optional.of(artifact);
+            }
+        }
+        return Optional.ofNullable(storeArtifact(artifactUpload));
+    }

- fix bug: change sql-query in localArtifactRepository #820

- fix bug: change sql-query in localArtifactRepository #820

Conversation

nazifebasbaz commented Apr 9, 2019

hawkbit bot commented Apr 9, 2019

Choose a reason for hiding this comment

nazifebasbaz commented Apr 25, 2019

a-sayyed May 14, 2019 • edited Loading

Choose a reason for hiding this comment

a-sayyed May 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-sayyed May 14, 2019 • edited Loading

Choose a reason for hiding this comment

a-sayyed May 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkbit-bot commented May 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bogdan-bondar left a comment

Choose a reason for hiding this comment

a-sayyed May 14, 2019 •

edited

Loading

a-sayyed May 14, 2019 •

edited

Loading

a-sayyed May 14, 2019 •

edited

Loading

a-sayyed May 14, 2019 •

edited

Loading