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

[WIP][REST] New API to transform to/from DSL file format (items & things) #4630

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

lolodomo
Copy link
Contributor

@lolodomo lolodomo commented Mar 4, 2025

Related to #4585

POST /file-format/parse to parse file format
POST /file-format/create to create file format

@lolodomo lolodomo requested a review from a team as a code owner March 4, 2025 20:36
@lolodomo lolodomo changed the title [REST] New API to create and parse file formats [WIP][REST] New API to create and parse file formats Mar 4, 2025
@lolodomo
Copy link
Contributor Author

lolodomo commented Mar 4, 2025

This PR is not yet ready for review because not yet finished. I consider it finished at around 75%.
I submitted it only because @mherwege wanted to know how it looks like.

@lolodomo lolodomo force-pushed the fileformat_api_for_ui branch from 18df1df to 41f606c Compare March 4, 2025 20:54
@lolodomo lolodomo changed the title [WIP][REST] New API to create and parse file formats [WIP][REST] New API to create and parse DSL file format Mar 4, 2025
@lolodomo lolodomo force-pushed the fileformat_api_for_ui branch 2 times, most recently from f4e1437 to 70b9979 Compare March 4, 2025 20:56
@lolodomo lolodomo changed the title [WIP][REST] New API to create and parse DSL file format [WIP][REST] New API to transform to/from DSL file format Mar 4, 2025
@spacemanspiff2007
Copy link
Contributor

It seems you decided to use the following naming schema:

 /file-format/{file-format-name}/parse
 /file-format/{file-format-name}/create

I think this is very good and I like it very much.

However for file-format-name you chose things and items.
These are the names for the core objects and not the file format and thus confusing.
I think it would be better if we use items-file and things-file.
What do you think?

@lolodomo
Copy link
Contributor Author

lolodomo commented Mar 5, 2025

/file-format/items is IMHO obvious that we are talking about file format for items. /file-format/items-file is redundant IMHO.. But if an official reviewer thinks your suggestion is better than mine, I will then change.

@spacemanspiff2007
Copy link
Contributor

But GET /file-format/items will return the existing items in a certain file format.
There the name is used to indicate that one is accessing the object item and not the file format items.
That's very confusing:

GET  /file-format/items       <-- items mean openHAB items
POST /file-format/items/parse <-- items mean *.items file

@mherwege
Copy link
Contributor

mherwege commented Mar 5, 2025

I don't want to steer this discussion too much and I would be fine with any conclusion on this, but here is another alternative that could be considered:

POST /file-format/parse
POST /file-format/create

As the 'what' is already in the MIME type (e.g. text/vnd.openhab.dsl.item), it is not strictly required here. And it also makes it possible in the future to just use something like text/vnd.openhab.dsl for a to be defined format that wraps multiple concepts in one file, without creating another end point.

@spacemanspiff2007
Copy link
Contributor

spacemanspiff2007 commented Mar 5, 2025

For parse the input is always a string and the output schema could be set according to the MIME type (thank you @mherwege for showing that that's possible).
For create the output is always a string and the input schema could be set according to the MIME type.

That would also be fine with me.

@lolodomo
Copy link
Contributor Author

lolodomo commented Mar 5, 2025

For output, as type is Response, there to s no problem to return any DTO object inside the Response.
But for input (provided as body), I still don't know how you define the type of the input parameter for the method as it can be ItemXXXDTO or ThingXXXDTO depending on content type header

@lolodomo
Copy link
Contributor Author

lolodomo commented Mar 5, 2025

@mherwege : the day we have a YAML file format, media type will be application/yaml. This is not sufficient for the parse API to know if it should return a ItemXXXDTO or ThingXXXDTO.. Not sufficient to specify what kind of information you are requesting.

@lolodomo
Copy link
Contributor Author

lolodomo commented Mar 5, 2025

Or you imagine to rather define text/vnd.openhab.yaml.item and text/vnd.openhab.yaml.thing ?

@spacemanspiff2007
Copy link
Contributor

I think it's important to differentiate between the existing ui yaml files and a possible new yaml format.
I suggest we somehow indicate that it's a ui yaml format.
So it would be text/yaml-ui.itemor text/yaml.ui.item or something like that.

@lolodomo
Copy link
Contributor Author

lolodomo commented Mar 5, 2025

I think it's important to differentiate between the existing ui yaml files and a possible new yaml format.

There is no plan to create 2 different YAML formats.

@spacemanspiff2007
Copy link
Contributor

My understanding is that you want to parse the already existing yaml from the ui code tab.
413338479-8bb8ef20-cfe4-4499-ad7b-2cd106199aa5
That would be the currently existing UI yaml with tow sub formats for item and thing.

Then there is also discussion about a new file format which contains more information and thus is not compatible to the ui yaml.
That would be the new config yaml (which still has to be properly defined and created).
This format should then be parsed, too.

The naming should make it clear which of the yaml formats you are referring to.

@lolodomo
Copy link
Contributor Author

lolodomo commented Mar 6, 2025

My understanding is that you want to parse the already existing yaml from the ui code tab.

No, this never was my intention. Please re-read the pointed issue you apparently missed the general intention. My intention is to show DSL and new YAML file formats in Main UI code tab. Management of YAML would be moved from Main UI to core. Main UI would just call API to create/parse YAML.

@lolodomo
Copy link
Contributor Author

lolodomo commented Mar 6, 2025

Then there is also discussion about a new file format which contains more information and thus is not compatible to the ui yaml.

The UI YAML will disappear in my solution and be replaced by any file format.

@spacemanspiff2007
Copy link
Contributor

Then that was a misunderstanding. I thought additionaly to the new yaml file the plan is to provide a parse function for the existing ui yamls so the forum examples will still work.

The new yaml format can contain both items and things at the same time.
Why would you need to differentiate between thing-yaml and items-yaml then?
That's why I thought they are the UI yamls because these are the only yamls where this differentiation exists.

@lolodomo
Copy link
Contributor Author

lolodomo commented Mar 6, 2025

Why would you need to differentiate between thing-yaml and items-yaml then?

Because UI need is to work either on item or thing, depending on the configuration WEB page.
But the YAML format will of course allow multiple kind of data. @J-N-K already provided an architecture allowing that easily.

One time again, we start discuss YAML in a PR which is not about YAML. Why not having this discussion rather in the issue itself?

@spacemanspiff2007
Copy link
Contributor

One time again, we start discuss YAML in a PR which is not about YAML. Why not having this discussion rather in the issue itself?

Because the way the format looks has implications and obviously we're not on the same page how the format looks.
For me the differentiation between a items and a things yaml makes no sense because the format makes it clear what is what and depending on the input it's clear what should be generated.

So please let's have this discussion (not here) otherwise it will be an endless source of confusion

@mherwege
Copy link
Contributor

mherwege commented Mar 6, 2025

Or you imagine to rather define text/vnd.openhab.yaml.item and text/vnd.openhab.yaml.thing ?

That was my thinking, but I am fine either way. You could then also have text/vnd.openhab.yaml which is a wrapper format that has it defined on a higher level with lists for things, items... So depending on the MIME type consumed, the outcome would be different. It has the disadvantage that these custom MIME types do not automatically get documented though. So I am on the fence.

For multiple types of objects content, if you want some analogies in the UI, we currently support working with this on multiple levels in the UI in a few places:

  • Pages: you can look at and edit the code of a page, which is a YAML structure that includes the full definition of the page with all its components, but you can also look at and modify the YAML code on an individual component level.
  • Rules: depends a bit on the rule type, but mostly the rule code page is a YAML with all triggers, actions (UI defined) and conditions, and if you have an inline script as an action, you will see the code under script

I would not get stuck on the exact format in the UI. That is a later concern. For now, I fully agree with @lolodomo that the first target should be making DSL import work without the parser code in the UI, just by calling a REST endpoint.

For output, as type is Response, there to s no problem to return any DTO object inside the Response.
But for input (provided as body), I still don't know how you define the type of the input parameter for the method as it can be ItemXXXDTO or ThingXXXDTO depending on content type header

I believe you can define multiple possible input MIME types as well in the @Consumes annotation and use @RequestBody to document it. I called in ChatGPT for some help:

@POST
@Path("/upload")
@Consumes({MediaType.APPLICATION_JSON, MediaType.APPLICATION_XML})
@Produces(MediaType.APPLICATION_JSON)
@Operation(summary = "Upload data", description = "Accepts JSON or XML input")
@ApiResponses(value = {
    @ApiResponse(responseCode = "200", description = "Success"),
    @ApiResponse(responseCode = "400", description = "Invalid input")
})
public Response uploadData(
    @RequestBody(description = "Input data", required = true, content = {
        @Content(mediaType = "application/json", schema = @Schema(implementation = MyData.class)),
        @Content(mediaType = "application/xml", schema = @Schema(implementation = MyData.class))
    }) MyData data) {
    return Response.ok().entity("Data uploaded").build();
}

@lolodomo
Copy link
Contributor Author

lolodomo commented Mar 6, 2025

In you example, the type is the same in both cases, MyData.
But ok, we can create a new MyData class that contain a collection of items and a collection of things. That was I believe more or less what @spacemanspiff2007 also suggested.
In that case, I agree we don't need to mention what is as input or what to provide at output when the file format supports both items and things (like YAML in the future(. As DSL format cannot contain items and things in the same fle, we need two different MIME types to distinguish items and things So we should keep text/vnd.openhab.dsl.item and text/vnd.openhab.dsl.thing and in the future add only text/vnd.openhab.yaml.
So yes I am going to switch to these unique APIs:
POST /file-format/parse
POST /file-format/create

@mherwege
Copy link
Contributor

mherwege commented Mar 6, 2025

You can achieve that with something like this:

@POST
@Path("/upload")
@Consumes({MediaType.APPLICATION_JSON, MediaType.APPLICATION_XML})
public Response uploadData(@Context HttpHeaders headers, String rawData) {
    String contentType = headers.getHeaderString(HttpHeaders.CONTENT_TYPE);

    if (MediaType.APPLICATION_JSON.equals(contentType)) {
        // Process JSON input
    } else if (MediaType.APPLICATION_XML.equals(contentType)) {
        // Process XML input
    } else {
        return Response.status(Response.Status.UNSUPPORTED_MEDIA_TYPE).entity("Unsupported format").build();
    }
    
    return Response.ok("Processed successfully").build();
}

@lolodomo
Copy link
Contributor Author

lolodomo commented Mar 6, 2025

For create API, content type will be only JSON and output will accept text/vnd.openhab.dsl.item and text/vnd.openhab.dsl.thing.
For parse API, content type will accept text/vnd.openhab.dsl.item and text/vnd.openhab.dsl.thing and output will be only JSON.

@lolodomo lolodomo force-pushed the fileformat_api_for_ui branch 2 times, most recently from a3d16f8 to 351a1e7 Compare March 7, 2025 19:04
@lolodomo lolodomo force-pushed the fileformat_api_for_ui branch from 351a1e7 to 561dc0b Compare March 8, 2025 11:14
@lolodomo lolodomo changed the title [WIP][REST] New API to transform to/from DSL file format [WIP][REST] New API to transform to/from DSL file format (items & things) Mar 8, 2025
@lolodomo lolodomo force-pushed the fileformat_api_for_ui branch 3 times, most recently from cb4bcc8 to 0bd3331 Compare March 8, 2025 17:54
Related to openhab#4585

POST /file-format/parse to parse file format
POST /file-format/create to create file format

Signed-off-by: Laurent Garnier <[email protected]>
@lolodomo lolodomo force-pushed the fileformat_api_for_ui branch from 0bd3331 to 7b6218b Compare March 9, 2025 09:30
@lolodomo
Copy link
Contributor Author

lolodomo commented Mar 9, 2025

Hi @mherwege , I defined this DTO object:

public class FileFormatDTO {

    public List<ItemDTO> items;
    public List<MetadataDTO> metadata;
    public List<ItemChannelLinkDTO> channelLinks;
    public List<ThingDTO> things;
}

but I was asking myself if it would be easier for Main UI to have channel links and metadata directly inside each item, rather than having them all grouped and separated ?

@lolodomo
Copy link
Contributor Author

lolodomo commented Mar 9, 2025

@mherwege : I also defined this class used as return type for the parse API:

public class ParsedFileFormatDTO extends FileFormatDTO {

    public List<String> warnings;
}

DSL parsers have two levels or errors, critical errors and warnings. Any critical error will reject the object loading, while warnings will not.

Regarding our parse API, any critical error detected by the DSL parser will lead to the API returning status code 400 with the errors as a message in the output.
In case there are no critical errors but warnings detected by the DSL parser, the API returns status code 200 but the output will contain the warnings as a list in warnings.
I believe it is a good solution. WDYT ?

@lolodomo
Copy link
Contributor Author

lolodomo commented Mar 9, 2025

It is now ready for things I believe.
Remains the retrieval of metadata & channel links when parsing .items.

@mherwege
Copy link
Contributor

mherwege commented Mar 9, 2025

Regarding our parse API, any critical error detected by the DSL parser will lead to the API returning status code 400 with the errors as a message in the output.
In case there are no critical errors but warnings detected by the DSL parser, the API returns status code 200 but the output will contain the warnings as a list in warnings.
I believe it is a good solution. WDYT ?

Yes, I think that’s a good solution in general. Can we get the line number and line position of the error(s) or warning(s) with this? That could allow the UI to point to the problem, as it does now. I don’t know if the DSL parser in core provides that info, or just strings for the errors and warnings.

@mherwege
Copy link
Contributor

mherwege commented Mar 9, 2025

but I was asking myself if it would be easier for Main UI to have channel links and metadata directly inside each item, rather than having them all grouped and separated ?

I have not gone through all UI code yet on this. It might be slightly more efficient if it is provided inside the item, but overall I don’t see a big issue in any case. The argument pro including it is that the REST endpoint for items also includes it I believe.

@lolodomo
Copy link
Contributor Author

lolodomo commented Mar 9, 2025

@spacemanspiff2007
Copy link
Contributor

spacemanspiff2007 commented Mar 10, 2025

The argument pro including it is that the REST endpoint for items also includes it I believe.

Yes - that's why I'd put it there, too.
Metadata in the file formats is always item bound so there's no use case for providing metadata without and item, in fact its most likely an error.
Imho metadata is only separate for historical reasons and today metadata and items are so intertwined, that this separation doesn't make any sense any more. So putting it in the item is the right way to do it.
Additionally this allows copy pasting rest responses without transformation.

For warnings, I am not sure:

That would require to return a json:

{
  "file": "FileText",
  "warnings": ["SomeWarningDto"]
}

I'm not really sure it it's a valid use case.
The format generation should always return a file that loads without warning and if the input is ambiguous it should raise an error.
Format parsing should always parse the file, warnings can be silently dropped there.
@mherwege do you have a specific case in mind where the warnings are of interest?

@lolodomo
Copy link
Contributor Author

The format generation should always return a file that loads without warning

We are talking about the other way.
There is no warning for file format generation.
There are warnings for file format parsing.

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