feat(java-sdk): Create Java Host SDK #122

bhelx · 2022-12-02T16:06:56Z

Closes #117

java/pom.xml

java/src/main/java/org/extism/sdk/LibExtism.java

We now resolve the extism library dynamically based on the rules defined in com.sun.jna.NativeLibrary. For tests we resolve the library relative to the cwd based on the jna.library.path system property set to ../target/release. Improves #117

- Refactor Plugin - Make `Plugin` and `Context` `AutoClosable` to automatically free resources in TWR blocks - Add missing javadoc - Upgrade to Junit 5 - Reorganize packages - Use more meaningful names - Introduce `ExtismException` - Make JSON serialization pluggable - Lower required java version to Java 17 Improves #117

thomasdarimont

Before finishing this, we could address the following points:

thomasdarimont · 2022-12-05T15:48:27Z

java/src/main/java/org/extism/sdk/manifest/Manifest.java

+    public List<String> allowedHosts;
+    public Map<String, String> config;
+
+    private static Gson _gson;


Suggested change

private static Gson _gson;

private static final Gson _gson;

thomasdarimont · 2022-12-05T23:07:27Z

java/src/main/java/org/extism/sdk/ExtismException.java

@@ -0,0 +1,15 @@
+package org.extism.sdk;
+
+public class ExtismException extends RuntimeException {


Need to add some javadoc here

thomasdarimont · 2022-12-05T23:07:55Z

java/src/main/java/org/extism/sdk/LibExtism.java

+     * @param pluginIndex
+     * @param json
+     * @param jsonLength
+     * @return


what does the boolean return mean?

same goes for what I wrote for #call

thomasdarimont · 2022-12-05T23:08:12Z

java/src/main/java/org/extism/sdk/LibExtism.java

+     * @param withWASI
+     * @return
+     */
+    boolean extism_plugin_update(Pointer contextPointer, int pluginIndex, byte[] wasm, int length, boolean withWASI);


What does the boolean return means?

same goes for what I wrote for #call

thomasdarimont · 2022-12-05T23:08:52Z

java/src/main/java/org/extism/sdk/LibExtism.java

+     *
+     * @param contextPointer
+     * @param pluginIndex
+     * @return


.. return the length of the output data in bytes.

thomasdarimont · 2022-12-05T23:11:43Z

java/src/main/java/org/extism/sdk/support/JsonSerializer.java

+    }
+
+    public static String toJson(Manifest manifest) {
+        return GSON.toJson(manifest);


We could remove the GSON dependency and implement a simple hand-crafted JSON serialization for the Manifest.

On the fence about it. I think eventually we will need to parse too. I'd be open to removing it but I can't say how long that will be feasible.

Quick idea:

Introduce a JsonConverter interface that can be used whenever we need to read / write json data. An instance of JsonConverter could be made available to the Plugin via the Context.

interface JsonConverter { <T> String toJson(T object); <T> T fromJson(String json, Class<T> type); }

Keep the gson library as dependency, but allow it to be excluded via dependency-excludes on the consumer side.

Implement a GsonJsonConverter implementation based on JsonSerializer

Introduce a ContextFactory where a JsonConverter can be configured centrally with a #setJsonConverter(..) method. Then offer something like ContextFactory#newContext() to create new Context classes.

With this in place, if users prefer another JSON library and don't want to pull in gson, they could exclude it with a maven dependency exclude and simply configure their own JsonConverter implementation, e.g. based on
fasterxml jackson.

Perhaps this looks a bit over-engineered, but I think having a ContextFactory to control creation of Context which could allow to share
some additional information might be useful.

Alternative:
Keep the JsonSerializer class as is and only use it internally. If we need to read json data, just add another method like:

public <T> T fromJson(String json, Class<T> object) { return GSON.fromJson(json, object); }

I think that's a good idea. I know how complex dependency clashing can get in Java. But I think it's fine to wait until after the first release. It should not affect the interface.

thomasdarimont · 2022-12-05T23:12:19Z

java/src/main/java/org/extism/sdk/wasm/ByteArrayWasmSource.java

+ * @param data the byte array representing the WASM code
+ * @param hash
+ */
+public record ByteArrayWasmSource(String name, byte[] data, String hash) implements WasmSource {


Do we need the hash? What hash should be used here?

thomasdarimont · 2022-12-05T23:12:44Z

java/src/main/java/org/extism/sdk/wasm/WasmSource.java

+ */
+public interface WasmSource {
+
+    String name();


Will every implementation of WasmSource have a String hash?

There is a json schema for the manifest and a page explaining the fields: https://extism.org/docs/concepts/manifest/

hash is optional.

Okay, I'll take a look.

thomasdarimont · 2022-12-05T23:13:32Z

java/src/test/java/org/extism/sdk/ExtismTest.java

+
+import static org.assertj.core.api.Assertions.assertThat;
+
+public class ExtismTest {


We need to add more tests for the remaining functions.

thomasdarimont · 2022-12-05T23:15:00Z

java/src/test/java/org/extism/sdk/ExtismTest.java

+    @Test
+    public void shouldInvokeNamedFunction() {
+
+        var wasmFile = Paths.get("src", "test", "resources", "code.wasm").toFile();


Instead of copying the code.wasm in the java project, we could also refer to "../wasm/code.wasm".

thomasdarimont · 2022-12-05T23:22:22Z

java/pom.xml

+  <groupId>org.extism.sdk</groupId>
+  <artifactId>extism</artifactId>
+  <packaging>jar</packaging>
+  <version>1.0-SNAPSHOT</version>


Should the version be aligned with the overall project version?

No, the Host SDKs will be versioned independently based on semver.

Okay, then I'd recommend to follow a semver approach and use 3 component version number.

thomasdarimont · 2022-12-05T23:23:06Z

java/pom.xml

+      <version>5.12.1</version>
+    </dependency>
+    <dependency>
+      <groupId>com.google.code.gson</groupId>


Do we actually need an JSON serialization library at all? If we only need to serialize the Manifest to JSON, perhaps a hand-crafted helper method will be sufficient.

thomasdarimont · 2022-12-05T23:24:51Z

java/src/main/java/org/extism/sdk/Context.java

+     * @param plugin
+     * @return
+     */
+    public String error(Plugin plugin) {


Perhaps an additional method like public String error() that explicitly returns the context error would be useful here.

this should be an internal method I think. I'm going to capture these errors then turn them into exceptions

Makes sense, then protected is fine, the error can be used as the message of ExtismException.

bhelx · 2022-12-06T18:02:06Z

@thomasdarimont in the past, i've published java libraries to OSSRH: https://central.sonatype.org/publish/publish-guide/#releasing-to-central

Is this still the best place to publish?

thomasdarimont · 2022-12-06T20:49:43Z

@bhelx Yes, releasing to maven central is still current, btw. the process of publishing releases can be simplified a lot by using a tool like jreleaser which makes publishing libraries to maven central a breeze.

thomasdarimont · 2022-12-06T21:21:49Z

I think it would also make sense to use a common source code formatter to format the code properly, otherwise a slightly diffenent IDE setup will cause formatting noise in pull requests.

Perhaps a tool like maven-checkstyle could help here.
A checkstyle example config with sentive values can be found here and here (maven).

thomasdarimont · 2022-12-06T21:25:13Z

java/pom.xml

+          <artifactId>maven-assembly-plugin</artifactId>
+          <configuration>
+              <descriptorRefs>
+                  <descriptorRef>jar-with-dependencies</descriptorRef>


It's usually recommended to let libraries pull in their dependencies explicitly, instead of creating a jar-with-dependencies.

thomasdarimont · 2022-12-06T21:26:00Z

java/pom.xml

+        </configuration>
+      </plugin>
+      <plugin>
+          <artifactId>maven-assembly-plugin</artifactId>


why is the maven-assembly-plugin needed at all? The jar builds fine without it.

thomasdarimont · 2022-12-06T21:27:58Z

java/pom.xml

+  <packaging>jar</packaging>
+  <version>1.0-SNAPSHOT</version>
+  <name>extism</name>
+  <url>https://github.com/extism/extism</url>


The project is lacking some metadata (organization, licensing, description, developer information) etc. that I think is necessary for releasing to maven central.

See here for a more complete library with complete maven metadata

bhelx · 2022-12-06T21:28:53Z

Good to know. It was a pain last time.

…

On Dec 6, 2022 at 2:49 PM -0600, Thomas Darimont ***@***.***>, wrote: @bhelx Yes, releasing to maven central is still current, btw. the process of publishing releases can be simplified a lot by using a tool like jreleaser which makes publishing libraries to maven central a breeze. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: ***@***.***>

thomasdarimont · 2022-12-07T22:39:40Z

I think I addressed most of the points I raised in the PR #147.
I kept the GSON dependency and did some additional refactorings.

thomasdarimont · 2022-12-07T22:42:32Z

For additional tests I need a few more example wasm files:

an example for reading a value from the manifest config and returning that as a result
a function that fails with an internal error

Perhaps you could create a generic "test" wasm file with multiple functions that could be used by all modules for consistent testing instead of having code.wasm in every sub-project.

- Context#error is now protected and used to extract the error message for the ExtismException - ByteArrayWasmSource is now correctly serialized (wasm data is now encoded base64) - Renamed ManifestMemory to MemoryOptions to align with rust codebase - Introduced WasmSourceResolver to load wasm source - Added unit tests - Revised Unit tests - Polished javadoc - Added necessary metadata for release to maven central - Removed maven-assembly-plugin as we want to allow consumers to control the dependencies

thomasdarimont · 2022-12-08T06:47:44Z

It seems as the ci fails due to unrelated issues in the Python SDK, perhaps a rebase with main would solve that.

thomasdarimont · 2022-12-08T21:57:36Z

Just added another PR: #150

I think the libray is now in good shape, however I recommend to address the ``FIXME` comments (in the non-test) code before releasing it.

I think the ManifestHttpRequest is not used at the moment and the code.wasm doesn't seem to have a function that can be used to test this. So we should either remove the class and add it later or add an example test and wasm file that uses the class.

thomasdarimont · 2022-12-09T19:51:24Z

Btw. the current state of the java-sdk requires, that users manually obtain the matching native extism library for their platform and store the library in a folder. This folder then needs to provided to the JVM via a System property e.g. -Djna.library.path=/path/to/folder. or the lib must be located in the OS specific library search paths.

This should be documented in the readme.md of the SDK.

Alternatively, we could package the matching library for various platforms directly in the jar and use a tool like native-lib-loader to load the proper library for the given OS dynamically. Would it be possible to provide the extism libraries for multiple os/platforms before the maven build runs?

How do we communicate to the user which version of the extism library is needed for the given java-sdk version?

bhelx · 2022-12-19T16:57:28Z

So this branch has been hanging our CI on the python jobs. We think it was just something with this branch and not the code. hoping that merging will fix the problem. If not we may revert

bhelx · 2022-12-19T17:38:07Z

Wow it's going green now: #175
turns out this branch was just haunted.

Thanks @thomasdarimont ! I'm going to work on docs now and some final cleanup items and push this out tomorrow

feat: Java Host SDK

c4ace19

bhelx commented Dec 2, 2022

View reviewed changes

java/pom.xml Outdated Show resolved Hide resolved

bhelx commented Dec 2, 2022

View reviewed changes

java/pom.xml Outdated Show resolved Hide resolved

bhelx commented Dec 2, 2022

View reviewed changes

java/src/main/java/org/extism/sdk/LibExtism.java Outdated Show resolved Hide resolved

bhelx and others added 6 commits December 2, 2022 13:11

Get count-vowels working, implement call

81eb02e

Support json manifest

dff16ae

feat: Java Host SDK (#132)

d025456

We now resolve the extism library dynamically based on the rules defined in com.sun.jna.NativeLibrary. For tests we resolve the library relative to the cwd based on the jna.library.path system property set to ../target/release. Improves #117

put assertion in test

fb3cd5c

Some more javadocs

58e5a1b

thomasdarimont suggested changes Dec 5, 2022

View reviewed changes

thomasdarimont reviewed Dec 5, 2022

View reviewed changes

thomasdarimont reviewed Dec 6, 2022

View reviewed changes

thomasdarimont and others added 3 commits December 7, 2022 20:07

Use oss@extism.org email

f7986b7

Use oss@extism.org email

a0b425f

Merge branch 'main' into java-sdk

ffb774e

bhelx and others added 13 commits December 14, 2022 11:13

change java setup requirements

d4e298c

fix yaml

c76f0fc

checkout action

913ca20

mvn verify, remove ld path set

06c3ac9

extism action

29c603d

remove try on the plugin

6edab66

implement logger

45e29e5

return int instead of pass in ref

db59b21

throw exception when index is -1

1273383

(java-sdk) Refactoring (#168)

9d04f7b

use error()

13efe9e

Merge branch 'java-sdk' of github.com:extism/extism into java-sdk

3fe2e79

cat the log

f94460b

bhelx force-pushed the java-sdk branch from ea563f3 to f94460b Compare December 15, 2022 22:43

bhelx added 5 commits December 15, 2022 17:07

print log failing step

fd78256

update action

7c1ecd2

print the json

ce212bd

comment out failing test

ac091e2

remove macos for java test, comment out extism logs

c0ea841

bhelx force-pushed the java-sdk branch from 4a852ef to c0ea841 Compare December 16, 2022 16:46

bhelx added 3 commits December 16, 2022 12:29

Merge branch 'main' of github.com:extism/extism into java-sdk

cde7d11

comment out java CI

f60f373

Merge branch 'main' of github.com:extism/extism into java-sdk

8f4f26e

nilslice previously approved these changes Dec 19, 2022

View reviewed changes

Add java back to CI

cd787a9

bhelx dismissed nilslice’s stale review via cd787a9 December 19, 2022 16:39

bhelx merged commit f34fa8b into main Dec 19, 2022

bhelx deleted the java-sdk branch December 19, 2022 16:55

		@@ -0,0 +1,15 @@
		package org.extism.sdk;

		public class ExtismException extends RuntimeException {


		import static org.assertj.core.api.Assertions.assertThat;

		public class ExtismTest {

feat(java-sdk): Create Java Host SDK #122

feat(java-sdk): Create Java Host SDK #122

Conversation

bhelx commented Dec 2, 2022 • edited Loading

thomasdarimont left a 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

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

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

bhelx commented Dec 6, 2022

thomasdarimont commented Dec 6, 2022

thomasdarimont commented Dec 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhelx commented Dec 6, 2022 via email

thomasdarimont commented Dec 7, 2022

thomasdarimont commented Dec 7, 2022

thomasdarimont commented Dec 8, 2022

thomasdarimont commented Dec 8, 2022 • edited Loading

thomasdarimont commented Dec 9, 2022 • edited Loading

bhelx commented Dec 19, 2022

bhelx commented Dec 19, 2022

bhelx commented Dec 2, 2022 •

edited

Loading

thomasdarimont commented Dec 8, 2022 •

edited

Loading

thomasdarimont commented Dec 9, 2022 •

edited

Loading