WIP: Model using jackson #8810

TarekkMA · 2021-05-12T15:00:53Z

Pull Request template

Purpose / Description

Describe the problem or feature and motivation

Fixes

Related to #8668

Approach

How does this change address the problem?

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration (SDK version(s), emulator or physical, etc)

Learning (optional, can help others)

Describe the research stage

Links to blog posts, patterns, libraries or addons used to solve this problem

Checklist

Please, go through these checks before submitting the PR.

You have not changed whitespace unnecessarily (it makes diffs hard to read)
You have a descriptive commit message with a short title (first line, max 50 chars).
Your code follows the style of the project (e.g. never omit braces in if statements)
You have commented your code, particularly in hard-to-understand areas
You have performed a self-review of your own code
UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
UI Changes: You have tested your change using the Google Accessibility Scanner

TarekkMA · 2021-05-12T15:05:36Z

a proof of concept PR to show how Jackson can be used to parse Model json.

Things that are highlited in this PR:

Parse array model.req into a class using @JsonFormat(shape= ARRAY)
Names of the fields are dependant from the name of the json
- bqfmt is now called mBrowserQuestionFormat and we use setBrowserQuestionFormat and getBrowserQuestionFormat to change it.
Unknown props will be retained and not lost
enum like classes (RequirementType, ModelType) that do save unknown values

Arthur-Milchior

enum like classes (RequirementType, ModelType) that do save unknown values

What do you mean by "unknown values"?
You can read more about requirement on https://github.com/Arthur-Milchior/anki/blob/commented/documentation/templates_generation_rules.md
I think it is not used anymore and will probably disappear with rust

Arthur-Milchior · 2021-05-12T20:20:42Z

AnkiDroid/src/main/java/com/ichi2/libanki/json/model/RequirementType.java

+    public static RequirementType[] values = new RequirementType[] {NONE, ALL, ANY};
+
+    @JsonCreator
+    public static RequirementType createFromValue(@Nullable String value) {


@Nullable RequirementType

I used @Contract("null -> null; !null -> !null")

Arthur-Milchior · 2021-05-12T20:23:47Z

AnkiDroid/src/main/java/com/ichi2/libanki/json/model/RequirementType.java

+    }
+
+
+    private final String mValue;


@NonNull String

Arthur-Milchior · 2021-05-12T20:24:09Z

AnkiDroid/src/main/java/com/ichi2/libanki/json/model/RequirementType.java

+        }
+        RequirementType that = (RequirementType) o;
+        return mUnknown == that.mUnknown &&
+                Objects.equals(mValue, that.mValue);


You can probably uses mValue.equals(that.mValue) since you know that mValue can't be null

Right, that was the auto-generated code from the IDE.

Arthur-Milchior · 2021-05-12T20:28:02Z

AnkiDroid/src/main/java/com/ichi2/libanki/json/model/Field.java

    @JsonProperty("ord")
-    private long mOrd;
+    private Long mOrd;


What the point of "move to object instead of primitives"
If we decide to go with Object, then this could be directly in the first commit I assume.

How did you test that it has the same performance?
Creating a Long object takes time. On long operation such as searching for empty card or cleaning collection, it's actually noticeable in the profiler.

You may be perfectly right, since all sqlite queries only take objects and no primitive, so we always convert long to Long when we need to send a query.

I did a benchmark between one implementation with primitives and the other with objects and they both are pretty much the same.

The benefit that we will have with objects is being able to set them to null, and Jackson will not include any null values to the outputted json.

while primitives can't be null.

If the idea of null is not very okay we can use Optional<T> for every field.

about the commits I will clean them once I make everything ready

Arthur-Milchior

Sorry, last comment was sent too early.

Thanks a lot for attacking this problem. I really appreciate it.

Did you try to use it really by replacing the models from libanki by your models?

I'm of two of mind.
On the one hand, it's extremely verbose. I don't like it. It make it hard to review because a single letter typo would not be easy to catch (that would be one good point of using static String instead of literals as I wanted to, but I digress). It also seems to add to the code complexity, while in fact almost everything is really simple here.

On the other hand, having getter/setter instead of accessing elements by String will be far more efficient, as there is not hashtable involved. I really really love it. Similarly, not having to deal with transforming checked exception into unchecked one will clearly be a great win!

Arthur-Milchior · 2021-05-12T20:30:57Z

AnkiDroid/src/main/java/com/ichi2/libanki/json/model/ModelType.java

+
+import androidx.annotation.Nullable;
+
+public class ModelType {


You probably would want to reuse com/ichi2/libanki/Model.java since it also represents a model

yes, this is the goal, but replacing the old model with the new one will result in many refactorings. so I'm saving it to the last step

TarekkMA · 2021-05-12T20:55:48Z

enum like classes (RequirementType, ModelType) that do save unknown values

What do you mean by "unknown values"?
You can read more about requirement on https://github.com/Arthur-Milchior/anki/blob/commented/documentation/templates_generation_rules.md
I think it is not used anymore and will probably disappear with rust

@Arthur-Milchior the goal is to not miss with the data if we don't know how to handle it. for example if have an enum defined in AnkiDroid with A, B we need the ability to preserve other unknown values like C, D. incase Anki desktop implemented a feature that is not yet found in AnkiDroid.

tbh my knowledge of how anki handles the low-level details is not good, I will try to read the attached link

Arthur-Milchior · 2021-05-12T21:05:25Z

You really don't have to read this link. It's probably deprecated and only used on old version of anki.

I understand better what you mean about "unknown", thanks. Nice to see you allow to accept any kind of value later!

TarekkMA · 2021-05-12T21:13:39Z

Yes it's very verbose, the idea behind adding annotation to every field is to make the field name independent of the json name.

and also becase the nature of java being verbose if we go with kotlin we would only have annotated constructor that's it.

we will get setter/getter copy (clone) method equals/hash code all for free

data class Model(
        @JsonProperty("id")
        var id: Long?,
        @JsonProperty("name")
        var name: String?,
        @JsonProperty("type")
        var type: ModelType?,
        @JsonProperty("mod")
        var mod: Long?,
        @JsonProperty("usn")
        var usn: Long?,
        @JsonProperty("sortf")
        var sortf: Long?,
        @JsonProperty("did")
        var did: Long?,
        @JsonProperty("tmpls")
        var tmpls: List<Template>?,
        @JsonProperty("flds")
        var flds: List<Field>?,
        @JsonProperty("css")
        var css: String?,
        @JsonProperty("latexPre")
        var latexPre: String?,
        @JsonProperty("latexPost")
        var latexPost: String?,
        @JsonProperty("latexsvg")
        var latexsvg: Boolean?,
        @JsonProperty("req")
        var req: List<Requirement>?,
        @JsonProperty("tags")
        var tags: List<Any>?,
        @JsonProperty("vers")
        var vers: List<Any>?,
        @JsonIgnore
        @get:JsonAnyGetter
        val mAdditionalProperties: MutableMap<String, Any> = HashMap()
) {
    @JsonAnySetter
    fun setAdditionalProperty(name: String, value: Any) {
        mAdditionalProperties[name] = value
    }
}

and we can use static constants inside annotations.

fixup! Create new Model class using jackson

TarekkMA · 2021-05-12T22:23:32Z

I have made the code less verbose, we don't need to annotate every setter and getter, annotating the field is enough I think.

dae · 2021-05-13T00:00:15Z

The latest Rust code exposes the various Anki objects like notetypes as typed protobuf messages, and notetypes are stored in the database as protobuf internally. While JSON-bound classes are a nice ergonomic improvement, given the changes coming with backend updates, I'm not sure it'd be worth the work involved to use them at the moment. Presumably you can use Jackson with the existing string-key API to get performance gains without requiring so many code changes?

TarekkMA · 2021-05-13T00:16:16Z

Thank you, Having the Anki objects saves as binary will be very nice since some JSON fields are more than 30 MBs. hopefully this will speed things

I will try to use jackson treemodel to replace the current json setup.

I noticed that much of the code in are depended on JSONObject/JSONArray object and even objects like Model and others are just subclasses of them. so lots of work are waiting in the future.

github-actions · 2021-07-12T00:51:32Z

Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically

Arthur-Milchior · 2021-07-12T02:41:57Z

Let us know @TarekkMA if you want us to take over it?

TarekkMA · 2021-07-13T15:39:08Z

@Arthur-Milchior Yes, thank you. Sorry I have some issues I'm dealing with 😞.

github-actions · 2021-09-11T16:45:39Z

Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically

TarekkMA marked this pull request as draft May 12, 2021 15:01

TarekkMA changed the title ~~wip models using jackson~~ WIP: Model using jackson May 12, 2021

TarekkMA force-pushed the jackson-model branch from 5c04bb3 to 669cbda Compare May 12, 2021 15:14

TarekkMA mentioned this pull request May 12, 2021

Investigating GSon #8668

Closed

Arthur-Milchior reviewed May 12, 2021

View reviewed changes

TarekkMA added 2 commits May 13, 2021 00:20

Create new Model class using jackson

d22010d

fixup! Create new Model class using jackson

Test de/serialization of Model

87852c5

TarekkMA force-pushed the jackson-model branch from 2ab139e to 87852c5 Compare May 12, 2021 22:21

github-actions bot added the Stale label Jul 12, 2021

github-actions bot removed the Stale label Jul 12, 2021

github-actions bot added the Stale label Sep 11, 2021

github-actions bot closed this Sep 18, 2021

Arthur-Milchior mentioned this pull request Apr 8, 2022

Improve JSON speed (probably with jackson) #10752

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Model using jackson #8810

WIP: Model using jackson #8810

TarekkMA commented May 12, 2021 •

edited

Loading

TarekkMA commented May 12, 2021 •

edited

Loading

Arthur-Milchior left a comment

Arthur-Milchior May 12, 2021

TarekkMA May 12, 2021

Arthur-Milchior May 12, 2021

Arthur-Milchior May 12, 2021

TarekkMA May 12, 2021

Arthur-Milchior May 12, 2021

TarekkMA May 12, 2021

TarekkMA May 12, 2021

Arthur-Milchior left a comment

Arthur-Milchior May 12, 2021

TarekkMA May 12, 2021

TarekkMA commented May 12, 2021

Arthur-Milchior commented May 12, 2021

TarekkMA commented May 12, 2021

TarekkMA commented May 12, 2021

dae commented May 13, 2021

TarekkMA commented May 13, 2021

github-actions bot commented Jul 12, 2021

Arthur-Milchior commented Jul 12, 2021

TarekkMA commented Jul 13, 2021

github-actions bot commented Sep 11, 2021


		import androidx.annotation.Nullable;

		public class ModelType {

WIP: Model using jackson #8810

WIP: Model using jackson #8810

Conversation

TarekkMA commented May 12, 2021 • edited Loading

Pull Request template

Purpose / Description

Fixes

Approach

How Has This Been Tested?

Learning (optional, can help others)

Checklist

TarekkMA commented May 12, 2021 • edited Loading

Arthur-Milchior 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

Arthur-Milchior left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TarekkMA commented May 12, 2021

Arthur-Milchior commented May 12, 2021

TarekkMA commented May 12, 2021

TarekkMA commented May 12, 2021

dae commented May 13, 2021

TarekkMA commented May 13, 2021

github-actions bot commented Jul 12, 2021

Arthur-Milchior commented Jul 12, 2021

TarekkMA commented Jul 13, 2021

github-actions bot commented Sep 11, 2021

TarekkMA commented May 12, 2021 •

edited

Loading

TarekkMA commented May 12, 2021 •

edited

Loading