openhab-core
openhab-core copied to clipboard
openhab
[REST] New API for conversion between file format and JSON
Related to #4585
This PR supports conversion of things and items. It supports DSL and YAML file formats.
A first new API (POST /file-format/create) allows to create a file format (DSL or YAML) from a JSON object. A second new API (POST /file-format/parse) allows to parse a file format (DSL or YAML) to a JSON object.
These 2 APIs should help Main UI displaying DSL and YAML file formats for things.
The Thing YAML /create only produced one line output:
version: 2
Fixed but I will have something to add for YAML output containing things and items. For your use cases in Main UI, it works as your are manipulating either an item or a thing.
Ok, I have now something working almost perfectly. All the difficulty was to parse syntax in an isolated model using the thing/item/... providers without impacting the thing/item/... registries. Remains one problem for DSL item to solve (related to state description). The solution could be to replace it by a stateDescription metadata delivering the pattern, it is what I did for YAML items, it makes no difference at runtime but has the advantage to make this format to be displayed by Main UI (as metadata) instead of being hidden.
@lolodomo I get that there's no time to do this now, but I've looked at how you've done this to make it possible to convert formats without them being registered, and I don't think the structure of the interfaces is "ideal".
We need to use this in other situations too, for example when installing (YAML based) add-ons from the marketplace. There should be a more "elegant" way into the system to turn these into an object. There's no reason for these to be "registered in the model", the model repository shouldn't need to "manage" them in any way, they just need to be "translated" into Java objects - so the "need" is much the same as with file format conversion.
I'm thinking that maybe the "model repository" should be uncoupled from the file watcher, so that it was generic and had "providers", where one of these providers could be the file provider. I don't have the full overview, so maybe that's not needed because "the model" is the set of configuration files that exists, it just feels somehow "wrong" that these are as tightly couples as they are.
But, the "redesign" I really think is needed is to make the "translation" uncoupled from the model repository. It should be possible to call "some entity" that is responsible for just converting (both to and from the format) that just returns the result of the conversion (and potential errors either as exceptions or in some other form). This "entity" should then be used by the model repository, and when the conversion is needed for other things, like for the API endpoints or the marketplace client.
I am not certain to clearly understand all your points but I just hope your intention is not to kill all the efforts already made and to avoid all these great new features to be part of OH 5.0.
The initial intention was to cover standard objects that are all in our model repositories since years, meaning items, things, ... Then my idea was to not reimplement a different parsing and generating code but rather to rely on existing code we already have in our model repositories (DSL and now YAML). It has the advantage to be simple and we are sure to have one unique set of rules to parse and generate file format
Maybe for your own need, this approach is not adapted because you don't have your objects already in the model repositories and you could probably implement something totally different with an extended API if necessary.
@lolodomo No, you misunderstood me completely. This isn't about "killing" anything, it's just about how the code is structured: What interfaces are defined, where/how things are implemented and called. I think that part needs some redesign, it wouldn't impact the functionality at all. What it would do is change the "Java API" (not the REST API) for other components wanting to communicate with these new components, like add-ons for example. But since it's all fresh, nothing should rely on it yet, so it should still be possible to redesign.
If the model repository was split from the "conversion" itself, there would be no need for "create isolated" and similar methods. You could just call the conversion directly, and so could the model repository.
It can be done later, so I can try to explain later when things are less chaotic.
edit: I'd like to add that I don't think it should be seen as "something wrong" if you have to redesign some parts as new needs arise. I typically do the same when I develop new stuff myself: I initially design it to suit the task I have in mind. As things progress, more things are added, and I'll often find out that now that initial design doesn't fit so well anymore. So, I redesign it to fit the purpose better. I see that as a part of "the natural evolution of the code", not as if I initially made a mistake because I didn't anticipate the other needs. It's "impossible" to start out with the full picture, or at the very least it would require an unreasonable amount of work to map out every possibility before development was started.
Is this still the "latest" code for YamlThing provider?
I found an issue using this branch: it doesn't check if a Thing with the same ID already exists before adding it to its "provider".
Steps:
- Create a Thing in Main UI, e.g.
astro:moon:test1 - Create a yaml file
version: 2
things:
astro:moon:test1:
config:
geolocation: "1,2"
- Edit the yaml file, and change the uid to
astro:moon:test2
You'll see
Provider 'YamlThingProvider' is not allowed to remove element 'ThingImpl' with key 'astro:moon:test1' from the registry because it was added by provider 'ManagedThingProvider'.
It should've logged a warning and refuse to add an existing thing id in the first place.
Was astro:moon:test1 already exiting before you start ? In your scenario, did you save in between? Did you show DSL syntax in between? Was the log generated when you saved your thing and called the API to create a new thing ? I don't understand what would trigger a removal of thing, is it a call from Main UI ? Which API is doing that?
Was astro:moon:test1 already exiting before you start ?
Yes. I created it through the UI first. After that I added the entry into a yaml file.
In your scenario, did you save in between?
Yes, of course.
Did you show DSL syntax in between?
No I didn't even try to "see" the Things on the UI. This is unrelated to the file-conversion thing, but I posted it here because I thought this PR contains the combination of all changes, so this is the branch I use for testing.
Was the log generated when you saved your thing and called the API to create a new thing ?
The error occurred when I changed the name (UID) to something else. This normally would've triggered the removal of the old thing and addition of a new thing with the new name.
I don't understand what would trigger a removal of thing, is it a call from Main UI ? Which API is doing that?
Follow the steps I wrote in the previous post. Yes, at the end of each step, I saved the file, otherwise it wouldn't change anything and the changes won't be registered.
The (failed) removal of a thing happened when I changed the thing's UID in the yaml file.
You first missed me because I thought it was when using the new API through the code tab You should create a separate issue as you are talking about something in the current distribution.
I understand the scenario now and I can try to reproduce it. The problem is probably when a thing is provided at the same time as UI and as config file. I have to check the checks in thing registry but I agree it would be better to have a message when first adding a thing that already existed through the UI managed provider. It is the load of the updated YAML file that triggers the thing removal in the registry.
Is it different when you use DSL thing ? I assume it is not.
@jimtng : I will have a serious rebase to do to take into consideration all changes done in other merged PRs.
The semantic tags editor in mainui also shows yaml code, so that is a candidate for a similar conversion to match yaml file syntax.
It should be ready now. I just have to make final tests to check everything is OK.
@openhab/core-maintainers and @jimtng : for your information, I finally consider that PR as completed and ready for a review.
Note that it is possible to convert things from YAML to DTO and from DTO to YAML even without the binding and associated thing types loaded. Same for DTO to DSL. But not for DSL to DTO. This is because I am re-using the parser of our existing thing providers and DSL provider does not load a thing without a thing handler. I will change that in a separate PR as this is something with more impacts than everything in that current PR.
Another fact is that the order of things when converting from YAML to DTO is not kept. I will have to investigate a little more.
Another fact is that the order of things when converting from YAML/DSL to DTO is not kept.
Probably a HashMap or HashSet is holding the data at some stage.
Another fact is that the order of things when converting from YAML/DSL to DTO is not kept.
Probably a HashMap or HashSet is holding the data at some stage.
Exactly, that is even at the core of our YAML loader (when the method listToMap is called):
https://github.com/openhab/openhab-core/blob/main/bundles/org.openhab.core.model.yaml/src/main/java/org/openhab/core/model/yaml/internal/YamlModelRepositoryImpl.java#L255
This probably concerns only YAML and not DSL. A test with two things in a DSL file seems to be ok, (same order in the generated DTO).
Can you just make listToMap use a LinkedHashMap instead? It might not be easy using "streams", but it's certainly easy enough to do the old-fashioned way.
Good idea, I will try. Current code is:
return elements.stream().collect(Collectors.toMap(YamlElement::getId, e -> e));
With good old-fashioned code, I would just do:
private Map<String, ? extends YamlElement> listToMap(List<? extends YamlElement> elements) {
LinkedHashMap<String, YamlElement> result = new LinkedHashMap<>(elements.size());
for (YamlElement element : elements) {
result.put(element.getId(), element);
}
return result;
}
I realise that we are finally handling elements of a YAML file in a random order and it would be more logic to respect the original order.
The current code could even be a problem for custom tags where the order is important. For items and things, I believe a load in a random order is in practice not a real problem.
I should even create a separate PR for that and we could then backport it to 5.0.x.
Even if it's not normally a problem, things like order tends to bite you when you least expect it, so I'd say it "safer" to do it in order even when no "known issue" with doing it in random order exists.
I agree. My PC is now shutdown, I will test with your proposed method tomorrow evening.