Skip to content

[REST] New API for conversion between file format and JSON #4793

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lolodomo
Copy link
Contributor

@lolodomo lolodomo commented May 7, 2025

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.

@lolodomo lolodomo requested a review from a team as a code owner May 7, 2025 15:57
@lolodomo lolodomo marked this pull request as draft May 7, 2025 15:57
@lolodomo lolodomo force-pushed the fileformat_create_parse_api branch 2 times, most recently from 14cdc00 to c299574 Compare May 7, 2025 16:00
@lolodomo
Copy link
Contributor Author

lolodomo commented May 7, 2025

This PR depends on #4776 . I will rebase it when #4776 is merged.

@jimtng
Copy link
Contributor

jimtng commented May 7, 2025

The Thing YAML /create only produced one line output:

version: 2

@jimtng
Copy link
Contributor

jimtng commented May 7, 2025

Items seem to be working fine for both YAML and DSL. Still testing further.

@lolodomo lolodomo force-pushed the fileformat_create_parse_api branch from 73057db to 5176977 Compare May 7, 2025 17:21
@lolodomo
Copy link
Contributor Author

lolodomo commented May 7, 2025

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.

@jimtng
Copy link
Contributor

jimtng commented May 7, 2025

Fixed

Thanks, it works now.

@lolodomo lolodomo force-pushed the fileformat_create_parse_api branch 7 times, most recently from b7f8433 to 4591c4f Compare May 9, 2025 22:21
@lolodomo lolodomo force-pushed the fileformat_create_parse_api branch from 4591c4f to c6bc278 Compare May 10, 2025 10:45
@lolodomo
Copy link
Contributor Author

lolodomo commented May 10, 2025

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 lolodomo force-pushed the fileformat_create_parse_api branch from c6bc278 to 630becf Compare May 10, 2025 11:09
@Nadahar
Copy link
Contributor

Nadahar commented May 11, 2025

@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.

@lolodomo
Copy link
Contributor Author

lolodomo commented May 11, 2025

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.

@Nadahar
Copy link
Contributor

Nadahar commented May 11, 2025

@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.

@jimtng
Copy link
Contributor

jimtng commented May 12, 2025

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.

@lolodomo
Copy link
Contributor Author

lolodomo commented May 12, 2025

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?

@jimtng
Copy link
Contributor

jimtng commented May 12, 2025

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.

@lolodomo
Copy link
Contributor Author

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
Copy link
Contributor

jimtng commented May 13, 2025

A separate issue created #4810

@lolodomo
Copy link
Contributor Author

@jimtng : I will have a serious rebase to do to take into consideration all changes done in other merged PRs.

Related to openhab#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.

Signed-off-by: Laurent Garnier <[email protected]>
@lolodomo lolodomo force-pushed the fileformat_create_parse_api branch from 630becf to 96af722 Compare May 18, 2025 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants