centralize the serialization/deserialization logic #36

niemannd · 2020-10-14T17:20:26Z

This PR provides a centralized json handling system.
The Idea is to use the same JsonHandler throughout the whole sdk, centralize the logic to serialize/deserialize and make it possible to change the underlying Json library without breaking changes.
This is the json pendant to #34

ATM the Code is not integrated anywhere. this would be topic for a future PR.

Usage would be

Movie movie = handler.decode("{"id":"1"}", Movie.class)
List<Movie> movieList= handler.decode("[{"id":"1"}{"id":"2"}{"id":"3"}]", Arraylist.class, Movie.class)

eskombro · 2020-10-14T18:09:48Z

I really like this 'abstract' vision that you are implementing, because to me it seems it can help with reducing strong dependencies and adding some possibilities of customization. But in this particular case, I was just wondering: does it happen often that you want to swap from one Json library to another one? I'm not against it at all, I'm just asking because I think this introduces more complexity in the code and want to be sure it is indeed useful. I am used to developing in languages where you have either a native way of handling JSON, or a library that kind of is the main choice for everyone, but I don't know how Java developers handle this in general, I see there are a few options that are used widely and don't seem to have a real consensus.

In the other hand, centralizing json handling seems like an amazing idea for usability and readability! 👍

Thanks @niemannd

niemannd · 2020-10-14T18:31:57Z

I [...] want to be sure it is indeed useful.

In the Java Ecosystem you have different userclusters that use different libraries.
The JakartaEE people use http://json-b.net/ and https://javaee.github.io/jsonp/
The Spring People use https://github.com/FasterXML/jackson
Some people like Gson, some use https://github.com/fangyidong/json-simple

To not scare away a possible userbase i wrote this little abstraction to allow them to integrate an existing json library. In some environments "just pull in a second json lib" doesn't fly and will get you in trouble. I hope its not that big of a braintwister 😄

eskombro · 2020-10-14T19:19:58Z

To not scare away a possible userbase i wrote this little abstraction to allow them to integrate an existing json library [...]

Thanks a lot! Your answer is quite reassuring, and it seems to be exactly what we want!!!

Then again, I ask myself ( and to you in here :) ) the same question than for other abstactions... should we make a choice to use a specific library by default, to make it easy to use out-of-the-box? With an abstraction like this one you are introducing, this kind of choice is a big help instead of a constraint! That's great!

I see you used gson, maybe because it was already used in this SDK, maybe because is your choice. But feel free to use a different one if you prefer

niemannd · 2020-10-17T07:41:10Z

I see you used gson, maybe because it was already used in this SDK, maybe because is your choice. But feel free to use a different one if you prefer

I added two additional Handlers.

JacksonJsonHandler that uses Jackson
JsonBJsonHandler that uses the JSON-B API with eclipse yasson as an implementation for the tests.
I never worked with JSON-B and yasson before, so use with caution. This handler is probably not bug free 😄

should we make a choice to use a specific library by default, to make it easy to use out-of-the-box?

Maybe we can automatically decide what implementation we use based on the classpath.
If Jackson is present, use JacksonJsonHandler. If Gson is present, use Gson.
Could look something like this:

Client
    .newBuilder()
    .withConfig(...)
    .withAutodetectHttpClient()  // for autodetect
    .withHttpClient(new ApacheHttpClient(...)) // for a specific implementation
    .withAutodetectJsonHandler()  // for autodetect
    .withJsonHandler(new JacksonJsonHandler(...))  // for a specific implementation
    .build();

niemannd · 2020-10-26T16:44:59Z

thats probably because the dependencies JacksonJsonHandler and JsonbJsonHandler need are compileOnly and not bundled with the dependency.

To use them your need the following dependencies in your build.gradle
For JacksonJsonHandler
compile group: 'com.fasterxml.jackson.core', name: 'jackson-databind', version: '2.11.3'
For JsonbJsonHandler
compile group: 'javax.json.bind', name: 'javax.json.bind-api', version: '1.0' and an implementation like eclipse yasson.
(in JavaEE/JakartaEE implementation should be provided by the application server, so only the api would be required)

eskombro · 2020-10-26T16:48:13Z

Oh sorry, I deleted the comment as I realized that! Thanks for your help!

Works smoothly 👍

eskombro · 2020-10-26T16:49:05Z

src/test/java/com/meilisearch/sdk/json/JsonbJsonHandlerTest.java

+	@Test
+	void serialize() throws Exception {
+		assertEquals("test", classToTest.encode("test"));
+		when(mapper.toJson(any(Movie.class))).thenThrow(new JsonbException("Oh boy!"));


eskombro

LGTM, great! 🌮

eskombro · 2020-10-26T16:50:44Z

Ready to merge @niemannd ?

niemannd · 2020-10-26T16:57:15Z

Ready to merge

eskombro · 2020-10-26T17:25:19Z

Another question, sorry...

Assuming

import com.meilisearch.sdk.Client;
import com.meilisearch.sdk.Config;
import com.meilisearch.sdk.UpdateStatus;
import com.meilisearch.sdk.Index;
import com.meilisearch.sdk.json.GsonJsonHandler;
import com.meilisearch.sdk.json.JacksonJsonHandler;
import com.meilisearch.sdk.json.JsonbJsonHandler;

final class Document {
  String id;
  String title;
}

final class Results {
  Document[] hits;
}

public class TestJava {
  public static void main(String[] args) throws Exception {

    GsonJsonHandler json_gson = new GsonJsonHandler();
    JacksonJsonHandler json_jackson = new JacksonJsonHandler();
    JsonbJsonHandler json_jsonb = new JsonbJsonHandler();

    final String indexUid = "books";
    final String documents = "["
      + "{\"id\": 123, \"title\": \"Pride and Prejudice\"},"
      + "{\"id\": 456, \"title\": \"Le Petit Prince\"},"
      + "{\"id\": 1, \"title\": \"Alice In Wonderland\"},"
      + "{\"id\": 1344, \"title\": \"The Hobbit\"},"
      + "{\"id\": 4, \"title\": \"Harry Potter and the Half-Blood Prince\"},"
      + "{\"id\": 2, \"title\": \"The Hitchhiker\'s Guide to the Galaxy\"}"
      + "]";

    Client client = new Client(new Config("http://localhost:7700", "masterKey"));
    
    Index index = client.createIndex(indexUid);
    UpdateStatus update = json_gson.decode(
      index.addDocuments(documents),
      UpdateStatus.class
    );

    index.waitForPendingUpdate(update.getUpdateId());

    // .... HERE GOES THE TEST
    // 
    // 
    // .... HERE GOES THE TEST

    client.deleteIndex(indexUid);

  }
}

If I test

Results res_gson = json_gson.decode(index.search("a"), Results.class);
System.out.println(res_gson.hits);
System.out.println(json_gson.encode(res_gson.hits));

I get the output:

[LtestJava.Document;@4034c28c
[{"id":"1","title":"Alice In Wonderland"},{"id":"123","title":"Pride and Prejudice"},{"id":"4","title":"Harry Potter and the Half-Blood Prince"}]

Then I try:

Results res_jackson = json_jackson.decode(index.search("a"), Results.class);
System.out.println(res_jackson.hits);

and the output is:

null

This seems to be a problem, can you check please?

niemannd · 2020-10-26T17:46:07Z

Thats a problem with the way you declared those classes.
Jackson can't serialize/deserialize package-private and private fields with default settings. Either make them public or (recommended) use Getters/Setters.

package com.meilisearch;

import com.meilisearch.sdk.Client;
import com.meilisearch.sdk.Config;
import com.meilisearch.sdk.Index;
import com.meilisearch.sdk.UpdateStatus;
import com.meilisearch.sdk.json.GsonJsonHandler;
import com.meilisearch.sdk.json.JacksonJsonHandler;
import com.meilisearch.sdk.json.JsonbJsonHandler;
import org.junit.jupiter.api.Test;

public class TestJava {

	@Test
	void name() throws Exception {

		GsonJsonHandler json_gson = new GsonJsonHandler();
		JacksonJsonHandler json_jackson = new JacksonJsonHandler();
		JsonbJsonHandler json_jsonb = new JsonbJsonHandler();

		final String indexUid = "books";
		final String documents = "["
			+ "{\"id\": 123, \"title\": \"Pride and Prejudice\"},"
			+ "{\"id\": 456, \"title\": \"Le Petit Prince\"},"
			+ "{\"id\": 1, \"title\": \"Alice In Wonderland\"},"
			+ "{\"id\": 1344, \"title\": \"The Hobbit\"},"
			+ "{\"id\": 4, \"title\": \"Harry Potter and the Half-Blood Prince\"},"
			+ "{\"id\": 2, \"title\": \"The Hitchhiker\'s Guide to the Galaxy\"}"
			+ "]";

		Client client = new Client(new Config("http://localhost:7700", "masterKey"));

		Index index = client.createIndex(indexUid);
		UpdateStatus update = json_gson.decode(
			index.addDocuments(documents),
			UpdateStatus.class
		);

		index.waitForPendingUpdate(update.getUpdateId());

		Results res_gson = json_gson.decode(index.search("a"), Results.class);
		System.out.println(res_gson.hits);
		System.out.println(json_gson.encode(res_gson.hits));

		Results res_jackson = json_jackson.decode(index.search("a"), Results.class);
		System.out.println(res_jackson.hits);
		System.out.println(json_jackson.encode(res_jackson.hits));

		client.deleteIndex(indexUid);
	}

	public static class Document {
		private String id;
		private String title;

		public String getId() {
			return id;
		}

		public Document setId(String id) {
			this.id = id;
			return this;
		}

		public String getTitle() {
			return title;
		}

		public Document setTitle(String title) {
			this.title = title;
			return this;
		}
	}

	public static class Results {
		private Document[] hits;

		public Document[] getHits() {
			return hits;
		}

		public Results setHits(Document[] hits) {
			this.hits = hits;
			return this;
		}
	}
}

eskombro · 2020-10-26T17:50:32Z

Great, sorry, as usual when you try to do things quickly you end up working double haha!

I'd like to add an integration test for this, using the 3 json implementations, and adding an assertion to check that the hits are the same no matter what encoder/decoder you use. Could you add it to the PR? I can commit a test if you prefer< but I prefer that everythiong is tested to avoid future problems :)

niemannd · 2020-10-26T18:17:44Z

I added unit tests for the other two JsonHandler.
At the moment i cant add an integration test, because the code is not really used.
In the future i would use something like this for integration tests.
Runs every encode/decode on all Handlers and compares the result. (probably still buggy)

public class MultiJsonHandler implements JsonHandler {
	private final JacksonJsonHandler jacksonJsonHandler = new JacksonJsonHandler();
	private final JsonbJsonHandler jsonbJsonHandler = new JsonbJsonHandler();
	private final GsonJsonHandler gsonJsonHandler = new GsonJsonHandler();

	@Override
	public String encode(Object o) throws Exception {
		final String encodeJackson = jacksonJsonHandler.encode(o);
		final String encodeJsonb = jsonbJsonHandler.encode(o);
		final String encodeGson = gsonJsonHandler.encode(o);

		assertThat(encodeGson, equalTo(encodeJackson));
		assertThat(encodeGson, equalTo(encodeJsonb));
		return encodeGson;
	}

	@Override
	public <T> T decode(Object o, Class<?> targetClass, Class<?>... parameters) throws Exception {
		T decodeJackson = jacksonJsonHandler.decode(o, targetClass, parameters);
		T decodeJsonb = jsonbJsonHandler.decode(o, targetClass, parameters)
		T decodeGson = gsonJsonHandler.decode(o, targetClass, parameters);
		assertThat(decodeGson, equalTo(decodeJackson));
		assertThat(decodeGson, equalTo(decodeJsonb));
		return decodeGson;
	}
}

eskombro · 2020-10-26T18:22:15Z

Amazing @niemannd 🚀 🚀 🚀 🚀

niemannd force-pushed the feat/json-handling branch from fbf420c to 2f270f1 Compare October 17, 2020 07:25

niemannd added 3 commits October 20, 2020 17:11

add json handling

c997542

add JacksonJsonHandler

36d76ec

add JsonB Handler

67cadb6

niemannd force-pushed the feat/json-handling branch from 2f270f1 to 67cadb6 Compare October 20, 2020 15:17

niemannd added 2 commits October 21, 2020 16:48

Merge branch 'master' into feat/json-handling

3d1ae75

apply Movie class changes

e5b4fdd

eskombro reviewed Oct 26, 2020

View reviewed changes

eskombro previously approved these changes Oct 26, 2020

View reviewed changes

add Unit Tests for jackson and Gson JsonHandler

214ba9d

niemannd dismissed eskombro’s stale review via 214ba9d October 26, 2020 18:13

eskombro merged commit e1bba35 into meilisearch:master Oct 26, 2020

curquiza mentioned this pull request Nov 25, 2020

Managing parameter input #3

Closed

niemannd deleted the feat/json-handling branch February 5, 2021 18:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

centralize the serialization/deserialization logic #36

centralize the serialization/deserialization logic #36

Uh oh!

niemannd commented Oct 14, 2020

Uh oh!

eskombro commented Oct 14, 2020 •

edited

Loading

Uh oh!

niemannd commented Oct 14, 2020

Uh oh!

eskombro commented Oct 14, 2020 •

edited

Loading

Uh oh!

niemannd commented Oct 17, 2020

Uh oh!

niemannd commented Oct 26, 2020

Uh oh!

eskombro commented Oct 26, 2020 •

edited

Loading

Uh oh!

eskombro Oct 26, 2020

Uh oh!

eskombro left a comment

Uh oh!

eskombro commented Oct 26, 2020

Uh oh!

niemannd commented Oct 26, 2020

Uh oh!

eskombro commented Oct 26, 2020 •

edited

Loading

Uh oh!

niemannd commented Oct 26, 2020

Uh oh!

eskombro commented Oct 26, 2020

Uh oh!

niemannd commented Oct 26, 2020

Uh oh!

eskombro commented Oct 26, 2020

Uh oh!

Uh oh!

centralize the serialization/deserialization logic #36

centralize the serialization/deserialization logic #36

Uh oh!

Conversation

niemannd commented Oct 14, 2020

Uh oh!

eskombro commented Oct 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

niemannd commented Oct 14, 2020

Uh oh!

eskombro commented Oct 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

niemannd commented Oct 17, 2020

Uh oh!

niemannd commented Oct 26, 2020

Uh oh!

eskombro commented Oct 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eskombro Oct 26, 2020

Choose a reason for hiding this comment

Uh oh!

eskombro left a comment

Choose a reason for hiding this comment

Uh oh!

eskombro commented Oct 26, 2020

Uh oh!

niemannd commented Oct 26, 2020

Uh oh!

eskombro commented Oct 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Assuming

If I test

I get the output:

Then I try:

and the output is:

Uh oh!

niemannd commented Oct 26, 2020

Uh oh!

eskombro commented Oct 26, 2020

Uh oh!

niemannd commented Oct 26, 2020

Uh oh!

eskombro commented Oct 26, 2020

Uh oh!

Uh oh!

eskombro commented Oct 14, 2020 •

edited

Loading

eskombro commented Oct 14, 2020 •

edited

Loading

eskombro commented Oct 26, 2020 •

edited

Loading

eskombro commented Oct 26, 2020 •

edited

Loading