CORE-17494: Add check to CPB creation to ensure that 2 CPKs don't share a name #596

malachyb · 2023-10-25T14:42:25Z

As part of #595, we are adding the ability for users to set an identifiable name for their own CPKs. However, we do not want 2 CPKs in the same CPB to have the same name, so we need to add a check in the CPB plugin to ensure that names are not reused within it.

simon-johnson-r3 · 2023-11-09T13:52:02Z

cordapp-cpk2/src/main/java/net/corda/plugins/cpb2/CpbTask.java

@@ -55,6 +58,8 @@ public CpbTask() {
            m.getAttributes().put(CPB_NAME_ATTRIBUTE, getArchiveBaseName());
            m.getAttributes().put(CPB_VERSION_ATTRIBUTE, getArchiveVersion());
        });
+
+        cpkNames = new HashMap<>();


I'm not a gradle expert, but having a static you reset on each instantiation of this type strikes me as a bit odd. Could you explain it more?

I'm not entirely sure why but for some reason when I don't reset it then it triggers the error. I think it might be persisting through multiple calls of the same plugin within the same project

I think what we are doing here (resetting it) suggests it should be made an instance property.

However it's interesting. I would expect this process to be one-off? I.e. pack CPKs intended for a single CPB and then exits? What more CPKs are there being processed/ output-ed by the prints added?

That fixed the issue, thanks

simon-johnson-r3 · 2023-11-09T13:56:23Z

cordapp-cpk2/src/main/java/net/corda/plugins/cpb2/CpbTask.java

+                System.out.println(cpkCordappName);
+                if (cpkNames.containsKey(cpkCordappName)) {
+                    if (cpkNames.get(cpkCordappName) == 1) {
+                        cpkNames.put(cpkCordappName, 2);


So you're allowing two cpks of the same name? I assume this is one for workflow and one for contract? Is it possible to make this check a bit more foolproof?

The plugin seems to iterate through each CPK twice or for some other reason calls this function twice on each CPK, so one instance of the name appearing will be counted twice

You should be able to call this function as many times as you like. Shouldn't cpkNames be local to this method?

Maybe it would help get to the bottom of this behaviour if you logged all the cpkNames? I don't think we can merge this without knowing what's going on.

kyriathar · 2023-11-17T14:17:04Z

cordapp-cpk2/src/main/java/net/corda/plugins/cpb2/CpbTask.java

@@ -61,18 +65,29 @@ public CpbTask() {
    @NotNull
    public AbstractCopyTask from(@NotNull Object... args) {
        return super.from(args, copySpec ->
-            copySpec.exclude(this::isCPK)


(this is how it used to be so nothing to do with the PR)

Not a Gradle expert but I think it looks to me a bit weird to exclude when isCPK.

kyriathar · 2023-11-17T14:20:41Z

cordapp-cpk2/src/main/java/net/corda/plugins/cpb2/CpbTask.java

+                } else {
+                    cpkNames.put(cpkCordappName, manifest);
+                }
+            }
            return cpkType != null && EXCLUDED_CPK_TYPES.contains(cpkType.toLowerCase());


(again before this PR)

the body of this function seems a bit misaligned with its name? The initial check (at line 73) does check if it's a CPK but then here we check if its Corda-CPK-Type (which I am not sure we currently use) is of of excluded cpk types?

It might make more sense if I split it into 3 functions that all get called by isValidCPK. Or maybe just 2 if Corda-CPK-Type isn't used anywhere?

Yes, I'd not spotted this before but

copySpec.exclude(this::isCPK)

seems to be excluding based on a predicate.

That would make "hijacking" this method fairly inappropriate for this same check. I'd revert this method to isCPK and make this logic it's own thing separate from that process.

kyriathar · 2023-11-17T14:22:17Z

cordapp-cpk2/src/main/java/net/corda/plugins/cpb2/CpbTask.java

+            Manifest manifest = cpkStream.getManifest();
+            String cpkType = manifest.getMainAttributes().getValue(CORDA_CPK_TYPE);
+            String cpkCordappName = manifest.getMainAttributes().getValue(CPK_CORDAPP_NAME);
+            if (cpkCordappName != null) {
+                if (cpkNames.containsKey(cpkCordappName)) {
+                    if (!cpkNames.get(cpkCordappName).equals(manifest)) {
+                        throw new InvalidUserDataException("Two CPKs may not share a cordappCpkName. Error in " + cpkCordappName);
+                    }
+                } else {
+                    cpkNames.put(cpkCordappName, manifest);
+                }
+            }


Perhaps we could try and extract this logic into some function named checkCordappNameUniqueness? This function is about checking whether it's isCPK/ isValidCPK. But this is about 2 CPKs sharing the same cordapp name (in the same CPB).

kyriathar · 2023-11-17T14:37:35Z

cordapp-cpk2/src/main/java/net/corda/plugins/cpb2/CpbTask.java

@@ -31,6 +34,7 @@ public class CpbTask extends Jar {
    public static final String CPB_VERSION_ATTRIBUTE = "Corda-CPB-Version";
    public static final String CPB_FORMAT_VERSION = "Corda-CPB-Format";
    public static final String CPB_CURRENT_FORMAT_VERSION = "2.0";
+    private final HashMap<String, Manifest> cpkNames = new HashMap<>();


Suggested change

private final HashMap<String, Manifest> cpkNames = new HashMap<>();

private final Map<String, Manifest> cpkNames = new HashMap<>();

kyriathar · 2023-11-17T16:34:26Z

cordapp-cpk2/src/main/java/net/corda/plugins/cpb2/CpbTask.java

@@ -31,6 +34,7 @@ public class CpbTask extends Jar {
    public static final String CPB_VERSION_ATTRIBUTE = "Corda-CPB-Version";
    public static final String CPB_FORMAT_VERSION = "Corda-CPB-Format";
    public static final String CPB_CURRENT_FORMAT_VERSION = "2.0";
+    private final HashMap<String, Manifest> cpkNames = new HashMap<>();


I think worth adding a short note on why we decided to map to Manifests

…rdappName

…g test to create 2 CPKs independently.

cordapp-cpk2/src/main/java/net/corda/plugins/cpb2/CpbTask.java

Co-authored-by: Kyriakos Tharrouniatis <[email protected]>

kyriathar

LGTM

malachyb marked this pull request as ready for review October 25, 2023 15:18

malachyb force-pushed the malachy/CORE-17494 branch from f5652f9 to ea0a90d Compare November 8, 2023 11:21

simon-johnson-r3 reviewed Nov 9, 2023

View reviewed changes

simon-johnson-r3 approved these changes Nov 16, 2023

View reviewed changes

kyriathar reviewed Nov 17, 2023

View reviewed changes

malachyb force-pushed the malachy/CORE-13259 branch from 6117527 to ea7f234 Compare December 6, 2023 10:33

kyriathar mentioned this pull request Dec 6, 2023

CORE-17493: Add cordappName field to contract and workflow fields of cpk2 #595

Merged

malachyb force-pushed the malachy/CORE-17494 branch 2 times, most recently from 512ea1f to 97623e0 Compare December 6, 2023 12:04

malachyb added 9 commits December 6, 2023 14:45

CORE-17494: Add check to CPB creation that two CPKs do not share a co…

2a80d53

…rdappName

Remove unused import

c58a82a

Fixed test

e7eece6

Adressed feedback by renaming CpbTask.isCPK to isValidCPK and changin…

1ebd3a2

…g test to create 2 CPKs independently.

Adressed feedback by converting CbpTask.cpkNames to an instance variable

5fd082b

Changed how CPB check works to instead verify against manifests

05bcf80

Updated error message to reflect new name of value

fe4ef2a

Move duplicate check out of exclude code

3d04b62

Removed unnecessary code

00df3d5

malachyb force-pushed the malachy/CORE-17494 branch from 97623e0 to 00df3d5 Compare December 6, 2023 14:51

kyriathar reviewed Dec 6, 2023

View reviewed changes

cordapp-cpk2/src/main/java/net/corda/plugins/cpb2/CpbTask.java Outdated Show resolved Hide resolved

cordapp-cpk2/src/main/java/net/corda/plugins/cpb2/CpbTask.java Outdated Show resolved Hide resolved

malachyb and others added 3 commits December 6, 2023 14:59

Update cordapp-cpk2/src/main/java/net/corda/plugins/cpb2/CpbTask.java

6805785

Co-authored-by: Kyriakos Tharrouniatis <[email protected]>

Update cordapp-cpk2/src/main/java/net/corda/plugins/cpb2/CpbTask.java

ab3e629

Co-authored-by: Kyriakos Tharrouniatis <[email protected]>

Fix variable renames

a12c9c8

kyriathar approved these changes Dec 6, 2023

View reviewed changes

malachyb merged commit 39f3341 into malachy/CORE-13259 Dec 6, 2023
5 checks passed

malachyb deleted the malachy/CORE-17494 branch December 6, 2023 15:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CORE-17494: Add check to CPB creation to ensure that 2 CPKs don't share a name #596

CORE-17494: Add check to CPB creation to ensure that 2 CPKs don't share a name #596

malachyb commented Oct 25, 2023 •

edited

Loading

simon-johnson-r3 Nov 9, 2023

malachyb Nov 9, 2023

kyriathar Nov 9, 2023

malachyb Nov 10, 2023

simon-johnson-r3 Nov 9, 2023

malachyb Nov 9, 2023

simon-johnson-r3 Nov 13, 2023

kyriathar Nov 17, 2023

kyriathar Nov 17, 2023

malachyb Nov 17, 2023

simon-johnson-r3 Nov 17, 2023

kyriathar Nov 17, 2023 •

edited

Loading

kyriathar Nov 17, 2023

kyriathar Nov 17, 2023 •

edited

Loading

kyriathar left a comment

	private final HashMap<String, Manifest> cpkNames = new HashMap<>();
	private final Map<String, Manifest> cpkNames = new HashMap<>();

CORE-17494: Add check to CPB creation to ensure that 2 CPKs don't share a name #596

CORE-17494: Add check to CPB creation to ensure that 2 CPKs don't share a name #596

Conversation

malachyb commented Oct 25, 2023 • 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

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

kyriathar Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyriathar Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

kyriathar left a comment

Choose a reason for hiding this comment

malachyb commented Oct 25, 2023 •

edited

Loading

kyriathar Nov 17, 2023 •

edited

Loading

kyriathar Nov 17, 2023 •

edited

Loading