ECR-4041] Update artefact representation by bullet-tooth · Pull Request #1349 · exonum/exonum-java-binding

bullet-tooth · 2020-01-08T15:56:35Z

Overview

See: https://jira.bf.local/browse/ECR-4041

Definition of Done

There are no TODOs left in the code
Change is covered by automated tests
The coding guidelines are followed
Public API has Javadoc
Method preconditions are checked and documented in the Javadoc of the method
Changelog is updated if needed (in case of notable or breaking changes)
The continuous integration build passes

bullet-tooth · 2020-01-08T15:56:53Z

re-created from #1341

dmitry-timofeev

It must be WIP — some code isn't even included (e.g., JavaArtifactNames.checkPluginArtifact).

dmitry-timofeev · 2020-01-09T07:39:12Z

    try {
-      JavaArtifactNames.checkArtifactName(pluginId);
-      return ServiceArtifactId.newJavaId(pluginId);
+      String[] result = JavaArtifactNames.checkPluginArtifact(pluginId);


The task description explicitly requests that pluginId is made equal to Exonum artifact id. https://jira.bf.local/browse/ECR-4041

bullet-tooth · 2020-01-09T13:23:59Z

-  void deployArtifact(String name, byte[] deploySpec) throws ServiceLoadingException {
-    DeployArguments deployArguments = parseDeployArgs(name, deploySpec);
+  void deployArtifact(byte[] artifactId, byte[] deploySpec) throws ServiceLoadingException {
+    ArtifactId artifact = parseArtifact(artifactId);


do we need to check here that artifact.runtimeId is java runtime?

I believe it is surely Core's responsibility

I'd say the core responsibility is to pass Java artifact ids only 🙃 But the check is redundant — Java Runtime loads only Java artifacts; if you pass non-Java artifact id later, it just won't be found.

dmitry-timofeev · 2020-01-10T07:40:12Z

  `IllegalArgumentException`.
 - Transaction index in block type changed from `long` to `int`. (#1348)
+- Extracted artifact version to the separate field from the artifact name.
+  Artifact name format is `groupId/artifactId` now. (#1349) 


I'd also specify the new full pluginId format, as it has also changed.

dmitry-timofeev · 2020-01-10T07:40:50Z

-import org.junit.jupiter.params.ParameterizedTest;
-import org.junit.jupiter.params.provider.ValueSource;

 class JavaArtifactNamesTest {


These tests no longer seem to have anything to do with artifact names, hence I suggest to rename the class and methods.

dmitry-timofeev · 2020-01-10T07:43:38Z

    return serviceDefinition;
  }

-  private static ServiceArtifactId extractServiceId(String pluginId)


Shan't we keep it to

Produce a more detailed error message,

Possibly, check that rid is 'Java'?

dmitry-timofeev · 2020-01-10T07:44:41Z

+    JavaArtifactNames.checkNoForbiddenChars(serviceArtifactId);
+    String[] coordinates = serviceArtifactId.split(DELIMITER, KEEP_EMPTY);
+    checkArgument(coordinates.length == 3,
+        "Invalid artifact id (%s), must have 'runtimeId:groupId/artifactId:version' format",


I'd say just 'runtimeId:serviceName:version' as we don't enforce groupId/artifactId

dmitry-timofeev · 2020-01-10T07:47:04Z

+    return serviceRuntime.isArtifactDeployed(serviceArtifact);
  }

  private static DeployArguments parseDeployArgs(String name, byte[] deploySpec) {


Pass full ArtifactId to keep all components in the log?

dmitry-timofeev · 2020-01-10T07:47:44Z

+    String name = "com.acme/foo";
+    String version = "1.2.3";
+    ArtifactId artifact = ArtifactId.newBuilder()
+        .setName(name)


Here and elsewhere: it does not set runtime id, which is unusual for core. I'd also consider extracting in a method (e.g., createArtifactIdMessage)

dmitry-timofeev · 2020-01-10T07:48:32Z

@@ -194,7 +195,7 @@
          <archive>
            <manifestEntries>
              <Plugin-Id>${artifactName}</Plugin-Id>


(here and elsewhere in services) PluginId is not correct (must be rid:name:version) — see the task desc.

dmitry-timofeev · 2020-01-10T07:49:19Z

@@ -224,6 +225,7 @@
          <systemPropertyVariables>
            <it.artifactFilename>${artifactFilename}.jar</it.artifactFilename>
            <it.artifactId>${artifactName}</it.artifactId>


It clashes with the standard project.artifactId, I suggest some other name (it.exonumArtifactId?)

dmitry-timofeev · 2020-01-10T07:50:19Z

          <systemPropertyVariables>
            <it.artifactFilename>${artifactFilename}.jar</it.artifactFilename>
            <it.artifactId>${artifactName}</it.artifactId>
+            <it.artifactVersion>${artifactVersion}</it.artifactVersion>


Why is version added? It does not seem to be needed — you must be able to take it from ^

dmitry-timofeev · 2020-01-10T08:45:44Z

I noticed in several places ArtifactId-proto is converted to ServiceArtifactId (SRA & SRA tests). Would it make sense to add ServiceArtifactId#from(ArtifactId)?

dmitry-timofeev · 2020-01-10T15:14:03Z

 * A service artifact identifier. It consists of the runtime id in which the service shall be
- * deployed and the service artifact name. The name of Java artifacts usually contains the three
- * coordinates identifying any Java artifact: groupId, artifactId and version.
+ * deployed and the service artifact name and the service artifact version.


Suggested change

* deployed and the service artifact name and the service artifact version.

* deployed, the service artifact name and its version.

dmitry-timofeev · 2020-01-10T15:14:48Z

+    ServiceArtifactId javaArtifactId = ServiceArtifactId
+        .newJavaId(artifact.getName(), artifact.getVersion());


ServiceArtifactId javaArtifactId = ServiceArtifactId.fromProto(artifact);

dmitry-timofeev · 2020-01-10T15:15:27Z

+        () -> ServiceArtifactId.parseFrom(artifactId));
+  }
+
+  private ArtifactId createArtifactIdMessage() {


Make it a constant?

Protobufs are immutable, this method is not parameterized.

dmitry-timofeev · 2020-01-10T15:16:14Z

-              <Plugin-Id>${artifactName}</Plugin-Id>
-              <Plugin-Version>${project.version}</Plugin-Version>
+              <Plugin-Id>${artifactId}</Plugin-Id>
+              <Plugin-Version>${artifactVersion}</Plugin-Version>


Probably, exonum.artifactId and same with version.

dmitry-timofeev · 2020-01-10T15:16:37Z

      return Runtime.ArtifactId.newBuilder()
              .setRuntimeId(artifactId.getRuntimeId())
              .setName(artifactId.getName())
+          .setVersion(artifactId.getVersion())
              .build();


dmitry-timofeev · 2020-01-10T15:26:34Z

+      "1:too-few:components:1.0",
+      "1:com.acme:foo-service:0.1.0:extra-component",
+      " : : ",
+      "1:com acme:foo:1.0",
+      "1 :com.acme:foo:1.0",
+      "1:com.acme:foo service:1.0",
+      "1:com.acme:foo-service:1 0",
+      "1:com.acme:foo-service: 1.0",
+      "1:com.acme:foo-service:1.0 ",


The sources need to be updated to reflect the new string representation.
E.g., "too-few:components:1.0" — is fine in terms of components; the problem here is non-integral runtime id.
Or, several "com.acme:foo service:1.0" with spaces do not necessarily trigger that as they, again,
have an invalid runtime id.

It still doesn't make sense — each element in the test data must have a single problem, and otherwise be valid. FTFY:

"", /* Too few components */ "too-few-components", "1:too-few-components", /* Extra component */ "1:foo-service:0.1.0:extra-component", /* All blanks */ " : : ", /* Non-integral runtime id */ "a:com.acme/foo:1.0", /* Spaces in runtime id */ "1 :com.acme/foo:1.0", /* Spaces in name */ "1:com.acme foo:1.0", "1:com.acme/fo o:1.0", /* Spaces in version */ "1:com.acme:foo: 1.0", "1:com.acme/foo:1.0 ", "1:com.acme:foo:1 0",

dmitry-timofeev · 2020-01-13T08:17:20Z

    new ServiceArtifactBuilder()
-        .setPluginId(serviceArtifactId.getName())
+        .setPluginId(serviceArtifactId.toString())
        .setPluginVersion(serviceArtifactVersion)


This method no longer needs a version — it can pull it from SAId.

bullet-tooth added work-in-progress 👷‍♂️ Do not expect reviewers to provide any feedback on WIP PRs — please ask for it explicitly! and removed work-in-progress 👷‍♂️ Do not expect reviewers to provide any feedback on WIP PRs — please ask for it explicitly! labels Jan 8, 2020

bullet-tooth requested a review from dmitry-timofeev January 8, 2020 16:06

dmitry-timofeev suggested changes Jan 9, 2020

View reviewed changes

vitvakatu force-pushed the new-protos-base branch from 893884d to a0a7acc Compare January 9, 2020 09:21

bullet-tooth added 4 commits January 9, 2020 13:31

change method signature

410f2ab

changelog+yml

2b5736f

fix merge conflicts

51196fd

apply changes

1b4c9c5

bullet-tooth added the work-in-progress 👷‍♂️ Do not expect reviewers to provide any feedback on WIP PRs — please ask for it explicitly! label Jan 9, 2020

fix plugin id

14e6ed9

bullet-tooth force-pushed the ecr-4041-new branch from 9ce8e94 to 14e6ed9 Compare January 9, 2020 13:20

bullet-tooth commented Jan 9, 2020

View reviewed changes

use artifact version in service poms

3a78677

bullet-tooth requested a review from dmitry-timofeev January 9, 2020 13:35

bullet-tooth removed the work-in-progress 👷‍♂️ Do not expect reviewers to provide any feedback on WIP PRs — please ask for it explicitly! label Jan 9, 2020

dmitry-timofeev suggested changes Jan 10, 2020

View reviewed changes

bullet-tooth added 2 commits January 10, 2020 15:42

PR comments fix

92267d0

add fromProto factory method

612b945

bullet-tooth requested a review from dmitry-timofeev January 10, 2020 14:02

bullet-tooth added 2 commits January 10, 2020 16:06

Merge branch 'new-protos-base2' into ecr-4041-new

f93030d

fix build

a18d8d9

dmitry-timofeev reviewed Jan 10, 2020

View reviewed changes

bullet-tooth added 4 commits January 10, 2020 18:05

fix tests

42d0c58

fix tests

61024ed

fix pr comments

862a900

fix pr comments

2fd43a0

bullet-tooth requested a review from dmitry-timofeev January 11, 2020 13:21

dmitry-timofeev approved these changes Jan 13, 2020

View reviewed changes

vitvakatu approved these changes Jan 13, 2020

View reviewed changes

fix plugin version setting in tests

8cdcc91

dmitry-timofeev approved these changes Jan 13, 2020

View reviewed changes

dmitry-timofeev merged commit 7b55c6e into new-protos-base Jan 13, 2020

dmitry-timofeev deleted the ecr-4041-new branch January 13, 2020 10:23

	* deployed and the service artifact name and the service artifact version.
	* deployed, the service artifact name and its version.

		ServiceArtifactId javaArtifactId = ServiceArtifactId
		.newJavaId(artifact.getName(), artifact.getVersion());

Conversation

bullet-tooth commented Jan 8, 2020

Overview

Definition of Done

Uh oh!

bullet-tooth commented Jan 8, 2020

Uh oh!

dmitry-timofeev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitry-timofeev commented Jan 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants