Initial implementation for JsonBindingService as discussed on #4741 #5856
Conversation
|
I also could not test it after rebasing from the master, as some model bundles seem to show errors... why is this happening, is there something wrong or outdated on my local setup?
These packages are completely unrelated and I never touched them, so I don't understand why I don't get a clean build with the files pulled from the master. |
There was a problem hiding this comment.
Thank you for taking a first step towards making it possible to change the JSON implementation.
You have touched our REST bundles to use your new service. Did you also have a look at other places which make use of Gson to see if your proposed API fits for them as well?
Regarding your compilation errors: First you should resolve the conflict in the one manifest file. Then, it might be that you started from a pretty old master and meanwhile things have changed in the model which should be generated again. Please run the Generate All Models.launch in the org.eclipse.smarthome.model.codegen project and afterwards clean all projects and let them build again.
Please have a look at my other comments in the code as well.
| @@ -0,0 +1,7 @@ | |||
| eclipse.preferences.version=1 | |||
There was a problem hiding this comment.
Please remove this preferences file, projects should use the workspace default settings.
There was a problem hiding this comment.
Removed, I think it was generated by Eclipse's "new project wizard"
| @@ -0,0 +1,3 @@ | |||
| eclipse.preferences.version=1 | |||
There was a problem hiding this comment.
Please remove this preferences file, projects should use the workspace default settings.
There was a problem hiding this comment.
Removed, I think it was generated by Eclipse's "new project wizard"
| @@ -0,0 +1,9 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
There was a problem hiding this comment.
Please remove this file and put a .gitignore file in the OSGI-INF directory which ignores /*.xml files so auto-generated files like this do not end up in the repository.
| <project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" | ||
| xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd"> | ||
|
|
||
| <modelVersion>4.0.0</modelVersion> |
There was a problem hiding this comment.
We are using spaces instead of tabs for indentations in pom.xml files, please change.
| * | ||
| * @return Created or previously existing Gson instance. | ||
| */ | ||
| protected Gson getGson() { |
There was a problem hiding this comment.
Having this only as protected will make the internal implementation, here Gson, leak out. Please make this private.
There was a problem hiding this comment.
This is protected to allow subclasses to overwrite this behavior if needed. This method is on the GsonBindingService class only, which is already Gson-specific. It does not appear in the interface.
| org.osgi.service.component, | ||
| org.slf4j | ||
| Service-Component: OSGI-INF/*.xml | ||
| Manifest-Version: 1.0 |
There was a problem hiding this comment.
Can you please check your formatter settings? The diff says you have changed the whole file although you probably have removed just one dependency.
There was a problem hiding this comment.
I have the default formatter settings, haven't really tweaked anything there. Not sure why this is happening: now I pulled the file again from the master, changed the single line I needed (for now, just adding the ...io.json bundle as a dependency, and it still seems to show up like that.
| * | ||
| * @author Simon Kaufmann - initial contribution and API | ||
| * @author Flavio Costa - Initial contribution and API |
There was a problem hiding this comment.
You are not the initial author. Please add your name with an @author tag below Simon and state what you have done, i.e. changing the implementation to use the JSON abstraction layer.
There was a problem hiding this comment.
Originally, I copied the GsonProvider class, created the new EntityProvider and then deleted the old GsonProvider. It looks like Git does not consider the new class to be a new/independent implementation, so now I adjusted the @author tag accordingly, keeping Simon as the original contributor.
| public GsonProvider() { | ||
| gson = new GsonBuilder().create(); | ||
| @Reference | ||
| void setJsonBindingLayer(JsonBindingService<T> binding) { |
There was a problem hiding this comment.
Usually the convention is to call the method after the property you want to inject, i.e. setJsonBindingService
There was a problem hiding this comment.
This was a leftover of an incomplete renaming, fixed.
| @Override | ||
| public long getSize(T t, Class<?> type, Type genericType, Annotation[] annotations, MediaType mediaType) { | ||
| return -1; | ||
| void unsetJsonBindingLayer(JsonBindingService<T> binding) { |
There was a problem hiding this comment.
This was a leftover of an incomplete renaming, fixed.
| /** | ||
| * Binding layer. | ||
| */ | ||
| private JsonBindingService<T> binding; |
There was a problem hiding this comment.
binding alone is a bit misleading in the ESH context because it is taken for external additions which add support for new hardware. Maybe call this jsonBinding or jsonService.
There was a problem hiding this comment.
Renamed to bindingService.
There was a problem hiding this comment.
Can't we split the bundle that defines the interface and the Gson specific implementation?
If I use an implementation for another JSON provider I would like to replace the GSON bundle with the other one but the interfaces someone uses to compile against needs to be the same.
| org.eclipse.jdt.annotation;resolution:=optional, | ||
| org.slf4j | ||
| Service-Component: OSGI-INF/*.xml | ||
| Bundle-ActivationPolicy: lazy |
There was a problem hiding this comment.
Which line, Bundle-ActivationPolicy: lazy? I see on #1912 it was discussed back in 2016, now I see it's no longer generating warnings, so it's removed.
| * @param object Object instance to be serialized. | ||
| * @param genericType Generic type definition. | ||
| * @param writer Writer where the Json output will be written to. | ||
| * @return Array of bytes containing the serialized Json. |
There was a problem hiding this comment.
Thanks, @return is removed from the Javadoc
flaviocosta-net
changed the title
|
@triller-telekom, thanks for the valuable feedback! Regenerating all models indeed fixed the errors in my workspace and allowed me to properly test the code. I've incorporated most of your comments, but it looks like I've erased the commit history when trying to rebase and resolve the conflict... Still, I will reply to your comments even if they now appear as obsolete. @maggu2810, separating the interface from the Gson implementation causes |
|
I will test the new API in more places to see if further changes are needed. One of my motivations here was to remove the Gson dependency from |
|
OK, now I have it being used in more places (where there are no hard dependencies on Gson or when the instance is not static), and while it seems to be working I get errors when running the tests: I haven't changed anything on org.eclipse.smarthome.core.items and that construction seems to exist, so why this is happening? Is this also the reason why Travis fails? |
|
Seems to be an unrelated error, but travis on this PR complains about one of your changes, have you seen that? |
|
@maggu2810, I am replying to your comment on #5884 here to keep the relevant history for this issue in one single place. We currently have Gson being used in different situations:
The case 1 is the ideal one in terms of system design, and it is completely covered by what I have developed here. However, it probably only reflect some 15-25% of the current Gson usages. To solve case 2, we either need to refactor code to promote the class to an OSGi component, so the dependency can be injected with DS or ServiceTracker. In cases this is not doable, we could still use Guice or another DI mechanism to remove the explicit dependency on Gson (it is ironic to use Guice to get rid of Gson, but we might need some alternative whenever OSGi injection is not available). This is mostly for 2a, the case 2b is simply evil and we should definitely refactor code to get rid of all those instances. Cases 3 and 4 are the more complicated, and it's difficult to know if we can actually provide a good solution that would eliminate all code that is using implementation-specific classes without creating something as complex as the original Json data binding component itself. The Rule deserialization issue seems to be one of these cases, where we would need to design some abstraction layer for FieldNamingStrategy, but in other places we would have to do something else that would allow us to write implementation-independent custom serializers, which also sounds complicated. What do you think about this? @kaikreuzer, do you think that we should proceed with this even if the outcome will only solve cases 1 and 2, and possibly some part of 3 and/or 4? We could progressively reduce the dependency on Gson, rejecting on the reviews any new Gson-dependent code if we already have a generic solution that replaces it. |
|
I agree with your points. |
|
While I like the overall goal to make implementations easily replaceable and use OSGi services, I am not fully convinced whether this effort is currently worth to pursue, considering the fact that it is quite a huge effort (and there are so many other important topics to address). As we currently do not have any plan to switch the implementation, what is our direct benefit?
Imho this will cause quite some pain. Many bindings are using GSON (as this has always been our advice) deep down in their internal implementation. Note that the binding API has been originally designed in a way that developers do not get in touch with OSGI very much (and thus they do not really have access to services with the sole exception of the HandlerFactories). We have already seen with the shared Jetty httpClient that it is quite inconvenient to pass the service to the places where it is needed and it blows up the code quite a bit. Replacing a GSON instance by a framework service would have a similar effect. So unless we are solving any current real issue with it, I would feel better with postponing this implementation/discussion until we have a real need. But this is of course just my humble opinion - if the rest of the committers wants to move on here, so be it. |
|
@kaikreuzer, I think there are some important trade-offs to consider here. I agree we may have a problem with lower priority / big effort / future benefit with this PR. However, if we don't do anything about this soon, then Gson will tend to become more and more used in all kinds of different places, and maybe become "irreplaceable" in the future. Json-B may be growing up, and become more attractive 2 or 3 years down the road. My suggestion for now would be to go for a "quick-win", meaning that we could try to do something that produces value without requiring too much effort, even if the solution is not perfect (i.e. doesn't solve 100% of the cases). This could be just implementing case 1 (which is mostly ready anyway) and 2 (shouldn't take more than a few hours), as proposed by @maggu2810. This would be beneficial, as we could start by removing/reducing Gson dependencies from "external" code pieces such as new ESH/OH bindings. It would also encourage proper OSGi-based designing, by obtaining the Json data binding service as an OSGi component, rather than keeping Gson instances in static fields as we see today... Yes, we would still need Gson-specific code to support cases 3 and 4, but this PR would already "fix" a significant number of occurrences we have today, and encourage better system designs for upcoming code. What do you think? |
|
Hey @flaviocosta-net, the main thing that worried me was the fact that I felt it is a step back wrt the ease of development for binding developers as getting hold of the json (de)serializer becomes much more complicated (as explained above). But after discussing it with @SJKA, we actually came up with an idea to solve this issue: We could extend the So let's move forward here - could you please rebase your branch in order to resolve the merge conflicts? |
|
@kaikreuzer, I will need to switch back to that branch to rebase the code, so before I do that I have two questions:
It sounds like a good idea to make certain components easily available to bindings, both to make developers' work easier and to help ensure consistency. Are you going to eventually provide this to all services listed under Optional Bundles? |
|
@flaviocosta-net I can answer 1. and 2. for you :)
Regarding which services we want to offer to binding developers is still open to discuss, I guess. |
|
Thanks for answering @triller-telekom, I have nothing to add.
Probably not. I actually already noticed an issue with my suggestion: We e.g. won't be able to offer the shared HttpClient this way, since the callback is defined in core.thing, which does not have a dependency on io.net, where the |
| <properties> | ||
| <bundle.symbolicName>org.eclipse.smarthome.io.json.gson</bundle.symbolicName> | ||
| <bundle.namespace>org.eclipse.smarthome.io.json.gson</bundle.namespace> | ||
| </properties> |
There was a problem hiding this comment.
Please remove these properties. See #6060.
| <properties> | ||
| <bundle.symbolicName>org.eclipse.smarthome.io.json</bundle.symbolicName> | ||
| <bundle.namespace>org.eclipse.smarthome.io.json</bundle.namespace> | ||
| </properties> |
There was a problem hiding this comment.
Please remove these properties. See #6060.
|
@flaviocosta-net Please let me know, if you need any further help wrt rebasing/updating this branch! |
|
Hi @kaikreuzer, sorry for the delay, I was trying to complete something for the new sitemap implementation before switching back to this, but now that you mention this I indeed need to ask you something:
This is the error I get when I try to complete an interactive rebase using EGit, after switching from the Do you have another suggestion, such as using a plain, non-interactive rebase or using command line instead of EGit? In this case, what would be the command to do it? Thanks! (my time will be used much more productively when I learn to use git properly...) |
Tie wisely spent 👍
This probably applies to most of us 😠 . |
|
@flaviocosta-net Let e know if you need any further help wrt git and getting this PR clean again! |
|
@kaikreuzer, I have performed the rebase (which Eclipse tells me to be completed successfully), removed the properties from the pom.xml files as requested by @wborn, and then tried to push the changes to the remote branch. I get a message apparently saying it was rejected due to not being fast-forward... what should be done with that? |
|
This is due to the rebase: The remote branch (base for this PR) is based on an older branch where several of the commits you now introduced by rebasing are missing. By default git refuses to "overwrite" the remote branch with this unknown changes. The rebase moved your own commits to the top of the commits list again, but the intermediate commits differ. |
flaviocosta-net
force-pushed
the
feature
branch
from
4e2f619
to
acfa088
Compare
|
@htreu, force-push worked well, thanks for the explanation! Now I see two conflicting files, but I don't see any actual difference in their content and rebase seems to have been successful... |
|
Hm, if you've rebased on the latest master, it should be impossible to have any conflicts. |
|
Please continue practicing your rebasing skills 😇 |
As you can see from the red upwards pointing arrow, this is only used to push to, but not to pull from.
The easiest solution would be to use the command line: These two lines should (1) create you a local branch with the name "feature" and (2) pull the commits from your remote fork. |
|
I could clone the remote branch, but I was having some difficulty to setup different URIs for fetch and push to support the triangular Git workflow we use. Now it seems to be correct, I could already do a rebase from the master. Right now, I am getting errors with several projects that do not build with |
I've never seen those, but it sounds like some bug in Eclipse... There are multiple Bugzilla entries (e.g. this one about that message. Try closing/opening/cleaning those projects and if all does not help, create a new workspace (again...). |
There was a problem hiding this comment.
I've never seen those, but it sounds like some bug in Eclipse...
Indeed, the issue eventually went away, thanks! I just did most of the minor improvements mentioned in the review. The major task now is to add a bundle for tests - I see there has been many changes to the test framework on the last months, is there a specific bundle that would be good to use as a basis to build that?
| */ | ||
| private final List<GsonTypeAdapterProvider> providers = new ArrayList<>(); | ||
|
|
||
| @Reference(cardinality = ReferenceCardinality.MULTIPLE) |
There was a problem hiding this comment.
OK, changing the reference to be DYNAMIC, thanks for the review.
As you say, this code is also used on so many places, so any changes here would be indirectly tested by other test bundles, but if you see the value I will create a minimal test bundle to accompany this one.
|
|
||
| @Activate | ||
| public void activate(Map<String, Object> properties) { | ||
| Boolean prettyPrinting = (Boolean) properties.get(FORMAT_OUTPUT_PROPERTY); |
There was a problem hiding this comment.
Originally I was having this on property files, but it sound overkill and it's something typically changed only for temporary debug purposes. It is now set on the @Component annotation on the class itself, so it is not something that would easily go wrong... but it would actually be nicer to change this to do the "formatted output" if log levels are set to DEBUG, but keep the unformatted Json output for INFO or less detailed logging levels. Do you know if this is something we can get from Logger or from somewhere else?
| @@ -0,0 +1,8 @@ | |||
| Manifest-Version: 1.0 | |||
| Bundle-ManifestVersion: 2 | |||
| Bundle-Name: Eclipse SmartHome JSON Data Binding Abstraction Layer | |||
| */ | ||
| @Provider | ||
| @Produces(MediaType.APPLICATION_JSON) | ||
| @Consumes(MediaType.APPLICATION_JSON) | ||
| @Component(immediate = true) | ||
| public class GsonProvider<T> implements MessageBodyReader<T>, MessageBodyWriter<T> { | ||
| @Component(immediate = true, service = { MessageBodyWriter.class, MessageBodyReader.class }) |
There was a problem hiding this comment.
If I remember correctly, I added these not because of OSGi, but because Jersey was not picking up the provider, but I can test again to confirm...
| </service> | ||
| <implementation class="org.eclipse.smarthome.binding.dmx.test.DmxTestHandlerFactory"/> | ||
| </scr:component> | ||
| <scr:component xmlns:scr="http://www.osgi.org/xmlns/scr/v1.1.0" immediate="true" name="binding.dmx.test"> |
There was a problem hiding this comment.
It's not related, seems to be a mistake in the commit.
| @@ -127,6 +127,8 @@ | |||
| <bundle>mvn:org.eclipse.smarthome.io/org.eclipse.smarthome.io.console/${project.version}</bundle> | |||
| <bundle>mvn:org.eclipse.smarthome.io/org.eclipse.smarthome.io.monitor/${project.version}</bundle> | |||
| <bundle>mvn:org.eclipse.smarthome.io/org.eclipse.smarthome.io.net/${project.version}</bundle> | |||
| <bundle>mvn:org.eclipse.smarthome.io/org.eclipse.smarthome.io.json/${project.version}</bundle> | |||
The main change was to remove Groovy from the tests - all tests should now be written in Java. P.S.: Note that you seem to have weird conflicts again, so time to rebase your branch once more... |
flaviocosta-net
force-pushed
the
feature
branch
2 times, most recently
from
dd6a90d
to
e44258b
Compare
|
@kaikreuzer, sorry, I should have made myself clearer: when I said "done", the changes were only committed to my local repo, I hadn't pushed them to the remote branch yet. I am now using |
Please also add it to the parent pom.xml so it gets build and tests executed during the mvn build run. |
|
I've added the test bundle to the parent pom.xml and executed it locally with success. Now Travis seems to be complaining about some missing dependencies on other packages... what exactly could be causing those errors? |
|
The error seems to be all the time: ( So, I am pretty sure (without looking at your changes) that you have added a package import of A feature itself should be complete WRT to requirement and capabilities. |
I did a Java search for all references to this package within the repository, and it just pointed to me to code on the bundle Maybe this is one of those weird Travis issues that go away with re-running it? Is the Travis re-execution something I am able to trigger on my own to validate if that is the case? |
|
I don't think it is related to Travis only. I see the same error if I try to build your branch. |
|
Hm, looking at the jar file it seems that core audio imports the respective package: (the forth beginning at the end) It has been introduced by: In the same commit the bundle provding that package has been added to the depending feature. You removed that bundle again from the feature (why?): |
flaviocosta-net
force-pushed
the
feature
branch
from
cde32d0
to
6cd9fc9
Compare
|
@flaviocosta-net I see you just did a force push two days ago, but the PR still seems to be broken (far too many files are touched in it and Travis fails right at the start). |
flaviocosta-net
force-pushed
the
feature
branch
from
e950e7f
to
d0998a9
Compare
@kaikreuzer, I believe it is fixed now, can you please review again and merge if good enough? |
|
Thanks, looks much cleaner already! |
| Bundle-RequiredExecutionEnvironment: JavaSE-1.8 | ||
| Bundle-SymbolicName: org.eclipse.smarthome.io.json.gson.test | ||
| Bundle-Vendor: Eclipse.org/SmartHome | ||
| Bundle-Version: 0.10.0.qualifier |
There was a problem hiding this comment.
This also needs to be changed to 0.11.0
| Bundle-ManifestVersion: 2 | ||
| Bundle-Name: Eclipse SmartHome JSON Data Binding Gson Implementation | ||
| Bundle-SymbolicName: org.eclipse.smarthome.io.json.gson | ||
| Bundle-Version: 0.10.0.qualifier |
There was a problem hiding this comment.
This also needs to be changed to 0.11.0
| Bundle-ManifestVersion: 2 | ||
| Bundle-Name: Eclipse SmartHome JSON Data Binding Abstraction Layer | ||
| Bundle-SymbolicName: org.eclipse.smarthome.io.json | ||
| Bundle-Version: 0.10.0.qualifier |
There was a problem hiding this comment.
This also needs to be changed to 0.11.0
|
@flaviocosta-net The static code analysis still fails with one error (and has one warning): |
Resetting the workspace and then adding the new bundles for a cleaner setup. Signed-off-by: Flavio Costa <flavio.costa.br@gmail.com>
flaviocosta-net
force-pushed
the
feature
branch
from
c3c4780
to
640ec5f
Compare
Signed-off-by: Flavio Costa <flavio.costa.br@gmail.com>
|
Hello @kaikreuzer, I have just fixed these two issues you mentioned, but I am not sure where you find them. What I see in Travis is this:
Does this indicate a reference to a bundle that is missing somewhere? |
|
Don't worry, @flaviocosta-net. |
|
Moved the reply to openhab/openhab-core#515 (comment) |
This PR aims at provide a new bundle that will abstract the JSON data binding implementation used for serialization and deserialization, so if in the future we want to change the concrete implementation it just needs to be changed in one place.
Right now it is using Gson, but it should be easy to use JSON-B/Yasson instead, if the problems reported on #4741 are resolved.
This PR also replaces the existing GsonProvider class in org.eclipse.smarthome.io.rest.core with a new EntityProvider which uses the new abstraction layer, thus is not directly dependent on Gson.