Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial implementation for JsonBindingService #515

Closed
wants to merge 2 commits into from

Conversation

kaikreuzer
Copy link
Member

@kaikreuzer kaikreuzer commented Feb 1, 2019

Ported over from eclipse-archived/smarthome#5856

Also-By: Flavio Costa flavio.costa.br@gmail.com
Signed-off-by: Kai Kreuzer kai@openhab.org

Also-By: Flavio Costa <flavio.costa.br@gmail.com>
Signed-off-by: Kai Kreuzer <kai@openhab.org>
@kaikreuzer kaikreuzer changed the title [WIP] Initial implementation for JsonBindingService Initial implementation for JsonBindingService Feb 1, 2019
@hakan42
Copy link
Contributor

hakan42 commented Feb 3, 2019

Many binding consuming JSON data from upstream API servers import and use the annotation com.google.gson.annotations.SerializedName to really clamp down the attribute naming.

How would this work if the specific json binding is abstracted away?

Signed-off-by: Kai Kreuzer <kai@openhab.org>
@kaikreuzer
Copy link
Member Author

@flaviocosta-net Can you answer @hakan42's question?

@maggu2810
Copy link
Contributor

I am missing an answer of this question for months, too.

See: eclipse-archived/smarthome#5884 (comment)
(starting some comments above: eclipse-archived/smarthome#5884 (comment))

@flaviocosta-net
Copy link

@kaikreuzer, before getting to the topic here, I'd like to thank you for the effort you've put into ESH until now (while you still had the Herculean amount of spare time needed to lead that)!

@hakan42 and @maggu2810, the idea was to consider this topic after the initial contribution was accepted (which never happened on the previous repository, largely because of the continuous issues I had with Travis or dependency resolution issues)...

Right now, there is no support for customized naming in this wrapper implementation but I see it must be supported as the feature is used in several places - we just need to agree on the design so I can test and implement something.

The reason why this JSON binding service even exists is because we failed to find a suitable implementation of the new JSON-B standard that does not rely on too many undesirable code dependencies. I think we may eventually remove this, and use JSON-B directly, but for now it may be useful as a first step to move away from Gson.

The JSON-B of doing this is either on the field, or on the get/set methods:

public class Person {
    @JsonbProperty("person-name")
    private String name;

    private String profession;
}

public class Person {
    private String name;
    private String profession;

    @JsonbProperty("name-to-write")
    public String getName() {
        return name;
    }

    @JsonbProperty("name-to-read")
    public void setName(String name) {
        this.name = name;
    }
}

Should we also have @JsonbProperty as the annotation name, assuming we want to make a migration easier in the future?

@maggu2810
Copy link
Contributor

Correct me if I am wrong, but if we want to use JSON in the code base, we will develop and compile against org.openhab.core.io.json instead of an specific implementation (e.g. org.openhab.core.io.json.gson).
So, we need to create an annotation in the org.openhab.core.io.json bundle ourself.

With that in mind, I don't care if the annotation is named JsonbProperty or different as long as the name is not too exotic. 😉
Regardless if the name match to the JSON-B used one or not, this is just a very simple refactor if you switch some time...

@hakan42
Copy link
Contributor

hakan42 commented Feb 6, 2019

So we would first rely on an org.openhab....JsonProperty first and then at some future time just change the import at the top of the class? I must admit that I am partial to the SerializedName annotation, but this is my day-in-and-day-out work with Spring which trained my mind into a specific direction 😁

The important part is really that we can decouple the class property names in the java class from the names in the serialized data stream.

@@ -0,0 +1,19 @@
This content is produced and maintained by the Eclipse SmartHome project.

* Project home: https://eclipse.org/smarthome/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update this to an openHAB NOTICE file.

@@ -0,0 +1,19 @@
This content is produced and maintained by the Eclipse SmartHome project.

* Project home: https://eclipse.org/smarthome/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update this to an openHAB NOTICE file.

@davidgraeff
Copy link
Member

Is that still being developed?

@kaikreuzer
Copy link
Member Author

I think this is waiting for a decision on #611 (comment)...

@maggu2810
Copy link
Contributor

Using official standards and to develop for APIs instead of specific implementation should always be preferred.
But #515 continues the specific ESH PR that has initial created on "Jul 4, 2018".

I don't know if the time that has been invested and needs to be further invested (development - review - cycles) makes sense.
If the PR is ready and just needs to be accepted, no need to turn it away.

So, how far are we from using a "official standard"?

@davidgraeff
Copy link
Member

davidgraeff commented Apr 12, 2019

javax.json is the official way :D Everyone is using jackson or gson out there, without abstraction layer. I don't understand why OH needs one.

@kaikreuzer
Copy link
Member Author

If the PR is ready and just needs to be accepted, no need to turn it away.

Well, it is ready, but it causes (functional) conflicts with #611, which are not easy to overcome.

If @flaviocosta-net is fine with it, I'd therefore suggest to close this PR despite all the effort that already went into this feature.

@kaikreuzer kaikreuzer closed this Apr 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants