-
-
Notifications
You must be signed in to change notification settings - Fork 432
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
Thing: move into core the conversions to/from YAML code done by MainUI #4585
Comments
If this initial step can be achieved, we can then discuss about small improvements of the syntax to make it potentially more user friendly and of course a compromise should be found. |
The existing API to update a thing expects at input either no channels and no change of channels for the thing is done in this case, or a list of channels and the thing will then be updated to have only this list of channels. So MainUI needs to maintain a list of all channels for a thing to call the update API. That might be the reason why all channels are shown in the code tab ? I am also asking myself if the purpose is also to let user remove channels ? That could be something preventing to have exactly the same YAML code if MainUI needs to display all existing channels while in a config file, the user would like to consider only non default channels. Even if the syntax itself remains common. |
Are you sure you wanted to ping me with this? :) I did not work on MainUI or openhab things in quite a while 🙈 |
No, it was a mistake, my intention was or course to ping @florian-h05 |
The proposed API are sufficient in general, it of course causes some network traffic but I think we can live with that.
a. I have only tried this for Things with unmodifiable channels, there it is possible to modify the channel config. In both cases, having them in the code tab is a great help. I think in b, it should be easy to keep the current behaviour because you actually need to define the channels manually, a is more difficult to handle though. |
Could it make sense to supply the mime type as part of the request instead of creating a whole new API endpoint? If the request includes "application/yaml" instead of "application/json" and it defaults to "application/json" could that work? DSL could use "application/dsl" and so on when/if new formats are developed. Then on the server side it knows what format is desired and can convert (as necessary) back and forth for new and updates to stuff. Or is that what you are already proposing (it doesn't seem so but I could be wrong)? Or is there something about doing that which wouldn't work?
I can confirm this, at least when it comes to MQTT and HTTP bindings.
My understanding is it just shows the raw JSON returned by the API converted to YAML. I don't think there is any more reasoning than that. And the raw JSON returned by the API includes all the Channels, as would be required to show all the Channels in the Design tab. To complicate a. a little more, what if I want to modify a Channel that I haven't modified before? There'd need to be a way to only show modified Channels and show all Channels and switch between the two. |
No because we have currently no API matching UI needs. I have now updated the new APIs in #4569 to limit the new APIs and to cover what is needed by MainUI. And in parallel, I already made a POC to implement YAML for things, it is relatively simple, not a lot of code lines because of great work done by @J-N-K on YAML registry after my initial contribution for YAML tags . Maybe I will include everything in #4569 . |
I thinks that's very unusual and would be both hard to teach and document e.g. in api explorer. |
Accept Media type is a HTTP header used to signal the server which media type the client accepts. It’s rather technical IMO and nothing to use to say I want format A or B. |
Just sliding in for this single comment, so sorry if this is out of context :) It's, from my experience, a workaround to represent different formats to serve by the server using a query Parameter. A much more standard way is in fact using Server-Side content-negotiation. This includes format, language and encoding, if supported by the server. See this MDN doc for it: https://developer.mozilla.org/en-US/docs/Web/HTTP/Content_negotiation It's also untrue that the So without being really involved here anymore, rather than having a query Parameter, using content negotiation would feel more naturally for an API, IMHO. |
I am not an API expert, but I must say I am using API's at work where the response is JSON or XML, depending on the |
The issue is that depending on the target format different input structures for a post request are required. |
I'm not sure I follow this completely, so my answer might be a bit off. Please correct me, if one of my assumptions is incorrect :) When POSTing to an endpoint, the This can then also be documented in an OpenAPI spec, something like this should do the trick, iirc: paths:
/things/{thingId}:
post:
requestBody:
required: true
content:
application/json:
schema:
$ref: '#/components/schemas/ThingUpdate'
text/yaml:
schema:
$ref: '#/components/schemas/ThingUpdate' (the API endpoint and anything else is just an example, it's merely to give an example about the doc :)) |
But when selecting a different file format the body for the request has to be different.
|
Yes, and you specify that with the So it is a combination of a If you keep the translation fuction within the item / thing endpoint, this would do it. If it would be a generic translation service, you will also need a path or query parameter to define what it is you are translating (e.g. item(s), thing(s), sitemap). I think this will avoid a proliferation of extra endpoints. |
But the To make a complete example: [{"name": "MyItemName", "label": "MyLabel"}] should return
and [{"UID": "MyThingName"}] should return
How will you document the required input structures in the openAPI spec if it's the same endpoint? |
I think the main difference here is the way how such an API would look like. E.g.: Variant 1: Making a
Doing the same request with a mime type in the
The very same thing would be done when updating a thing, just with a POST request. In that, the With this approach, there is no separate API endpoint to do conversion of structures, as the respective endpoints of the obejcts themself can do the necessary conversion out of the box. Variant 2: As you can see with this, using this approach in fact only duplicates the number of endpoints, as you would need to have a conversion endpoint per type, while you still need the GET and POST endpoints for the types anyway. I hope it makes sense what I want to express here :) |
I think we are talking past each other here. The input could also be:
and then then
Or the other way around, input
and
You will need to distinguish between what your are converting (items, things, sitemaps...), and that would be in the subpath or a query parameter. |
The reason for a separate endpoint would be to have the UI request the translated syntax without committing it to OH yet. Right now, the UI has its own set of parsers and generators to do that translation. And you are able to edit yaml in the UI (and DSL for sitemaps in the UI). You would need a way to switch between the full UI representation or whatever textual format without committing it yet. Having a REST endpoint to get the translation from core would allow to keep it all in sync in core and remove the parsers and generators from the UI. They are difficult to keep in sync as it is now and core can leverage what is already there. But it doesn't have to be one or the other. I can see value in having this on the item/thing endpoint, and also have a translation endpoint that does not commit. The UI is ultimately responsible for committing then. |
It's nice that you two are so eager to comment but both of you have not answered my question at all but rather made suggestions how something totally different could look like.
This is nice because on both api endpoints the request body structure can be properly described in the api docs. Both you suggest that the
and the
The use case where one wants to transform only one already existing item or all existing items is very limited.
That's why I wrote above:
|
This may be your goal. But I am looking at it from different perspectives. First look at the title of the issue: to/from YAML. It doesn't specify the other format you convert to/from. It talks about moving into core. So that assumes we are talking about JSON to/from YAML conversion, not DSL. Because the JSON/YAML conversion is currently what is done in the UI. There is no DSL to YAML conversion in the UI.
The related PR #4569 is just about generating DSL syntax for things/items. I want to look at it more holistically. We already have 3 formats here (JSON, YAML, DSL) and we want to convert to/from, without directly committing anything. And there might be other formats in the future. That's my base premise. So I think setting the input format and output format would be perfectly fine using
And that is exactly what you propose as well except that you always assume DSL as the output. There is no such thing as an item format or a thing format. There is a DSL format for items, a YAML format for items, a JSON format for items...
In this case, you cannot even rely on a subpath or parameter (or a totally separate endpoint). It has to be encoded in the file itself what the entity (entities) is (are) that you are converting. That would require an extension to the language syntax, some form of a wrapper array in the data itself. That's the only way you would be able to split it out. But you can still enforce the content type to be consistent for the whole call (if you define something in DSL, everything in it should be in DSL). |
I know that it's dsl and I know that there is YAML from UI. It's an example because the output is irrelevant for the point that there's no way to document the required input structure.
Why would it not work? In a subpath I can properly document the required input structure. I have tried multiple times to explain that my issue is the lack of documentation of the required structure (not format!). |
Sorry if I didn't understand what you are trying to say. So, correct me if I am wrong, but your concern is that the message body example and schema would be auto-generated for
Looking at the code, this documentation is auto-generated. So I don't think it is easy to do that for custom defined formats anyway. You can only have I can see the complication in coding, but I don't see why that should impact the REST endpoint structure. In the end, if the input is DSL or YAML, the best we have now is string as an input parameter. That does not auto-document either at all, even if you create a separate endpoint for it. So I would argue a translation API should just have string as input and output, but depending on the header set, interpret the content of the string differently.
Yes, maybe I am making the wrong assumption that the formats have all info and you can translate in all directions. Still, wouldn't that mean that?
And if that is possible, what is stopping you from internally converting to JSON (and that would not be what happens, it would convert to the DTO) and then to the requested format in the same call? |
I did a quick check, modifying one of the things endpoints. I can have multiple accept types and have different documentation by accept type, just by changing the annotations. Here is a little video illustrating it. In the video, the underlying DTO does not change, but that would be possible as well. |
I was not expecting such "intense" discussion already on the REST API itself... I let you find a conclusion, the API part is for me more or less a detail and a small part of my changes. During this time, I will finalize the changes to be sure that the POST APIs will not use at all the items/things registries. @mherwege : if several media types are proposed by the API as output, how to you define and annotate the method to let it know that the API will return a ThingDTO.class in case of media type "JSON", a String.class in case of media type "YAML" and a String.class in case of media type "DSL" ? How a java method could return different thing types ? Should I use Object ? Can you provide an example please ? Same question for POST (input) body, how do you define a "variable" input type ? I would also appreciate an example. Should I use generic |
But if you parse a file format that contains thing data as input you have to set the output DTO based on the request E.g.
My actual goal is simple: |
Have a look here: main...mherwege:openhab-core:yaml I patched the Note that all REST calls in the code return a Response object. What is being updated is purely internal to the method. This example is for a producer, but a consumer can be done in the same way. DSL is most likely much more tricky from a documentation perspective and I didn't investigate deeper. In the worst case, we should have to treat the whole DSL input/output as a String object. But it might be possible to do better. I didn't investigate further. |
No, it was not discussed yet to provide such new features. But I agree we can open a door for that at API level for the future. |
@mherwege : is it so obvious for you too ? |
They would be for managed items. Non-managed shows the same fields, but with the lock icon. So you are correct in this case. But for managed items, what is shown is editable.
I am fine with that. I agree. But I return the question. What is that canonical JSON format we are talking about? Is it the existing REST endpoints? Or is it the internal UI object structure that gets translated in one or multiple JSON’s for one or multiple REST calls? That object structure is sometimes also different by UI page. I see more use for it when converting to/from DSL, or for converting to a YAML alternative for DSL. I agree showing JSON in a code tab would not be very useful, although it is done for sitemaps (both DSL and JSON) on the same screen. But the UI uses YAML for a reason. It is more human readable. The YAML and internal object representation (visualized as JSON) are just another visualization of the same internal UI object.
Indeed, I stand correct the item GET includes metadata and links. Adding or updating requires 3 endpoints.
The item edit view in the UI does not include item links and metadata in the same place. So the YAML code view also does not have it. And the internal object representation does not have it. So there is a risk of losing information when you count on it being complete.
It doesn’t have to be a string. As I illustrated in the POC you could define a YAML output as such and have it fully documented as YAML.
I am OK with separate endpoints.
Agreed in the context of exporting existing things to a DSL file format, not in the context of the code view for editing a single thing (as it is now). The JSON also does not have the nesting.
The item import function already does that at the moment, but the UI parser is unable to cope with the metadata. And this is where @lolodomo work will be a big improvement, doing all that in core.
Yes, it is. I got carried away envisioning to use it also on existing endpoints, but I never questioned there was a need for separate endpoints.
|
The JSON we are talking about is the DTO objects we have in core that are used by our REST APIs. Remember that one of the initial wish is to have in Main UI code tab a YAML code that is the same as what will be used in the future as file format. The idea is not to have a specific YAML format for Main UI. All this discussion started with that assumption and @florian-h05 told us it was doable. So this is something to clarify quickly with @florian-h05 .
Until Main UI is enhanced to support channel links and metadata in code tab, Main UI could call the API with no channel link and no metadata so that they will not be displayed in code tab. But of course, in that case, what is displayed in code tab is not the full file format but rather a partial format.
Ok, I will have a look.
Perfect, I prefer to separate them.
But Main UI could call the new API providing an array of one unique JSON element (item or thing), no ? Generated YAML or DSL will then only contain one item or one thing.
Ok, so I am going to move the new APIs to a new separate /file-format endpoint as suggested by @spacemanspiff2007 . |
I'm aware, the json will return two separate thing definitions which is why a list is required.
That's my understanding, too. The individual objects are json DTOs of the core objects.
When parsing a format one will have the data in the correct format to feed it to the other endpoints. |
I think it must be possible as well, but a whole lot more complex. It is all about the automatic endpoint documentation, if you will have a documented data structure in swagger or not. That’s something that can always be refined later. |
See my answer in the PR. It is not complicated at all to give a proper example for DSL. |
I'll create an issue for this.
Or is there already an issue open for this? Or it's not needed?
This makes the code view of Items unique among OH entities. For consistency sake, if for no other reason, there's a strong argument to making a code tab show the code for everything you see on the Item's page which then should include everything that you can define for a DSL Item. Maybe we add a code tab to the main Item's page but keep the code tab when editing the Item and the metadata. The link is not represented with a code tab anywhere. |
@lolodomo Do you already have code to convert DSL to the internal JSON format? I really would like to get rid of the nearly parsers in the UI, as it duplicates the DSL to JSON conversion in the UI with a separate parser. That would replace the DSL items import nearly parser code, and would open the door for things, sitemaps and persistence as well. As you know, I am particularly interested in sitemaps as we have both a DSL nearly parser and a DSL generator in the UI, with all consequential difficulties to maintain it in sync. But I am waiting to see how it is done for the other formats. And replacing the items import by a call to a REST endpoint for the translation would be a first step. |
Related to openhab#4585 POST /file-format/items/parse to parse items from file format POST /file-format/items/create to create items in file format POST /file-format/things/parse to parse things from file format POST /file-format/things/create to create things in file format Signed-off-by: Laurent Garnier <[email protected]>
@mherwege : I submitted a WIP PR just to show the current state and how parsing is achieved. Everything is not yet ready, the retrieval of metadata when parsing DSL items is not yet done for example. |
Related to openhab#4585 POST /file-format/items/parse to parse items from file format POST /file-format/items/create to create items in file format POST /file-format/things/parse to parse things from file format POST /file-format/things/create to create things in file format Signed-off-by: Laurent Garnier <[email protected]>
Related to openhab#4585 POST /file-format/items/parse to parse items from file format POST /file-format/items/create to create items in file format POST /file-format/things/parse to parse things from file format POST /file-format/things/create to create things in file format Signed-off-by: Laurent Garnier <[email protected]>
Related to openhab#4585 POST /file-format/items/parse to parse items from file format POST /file-format/items/create to create items in file format POST /file-format/things/parse to parse things from file format POST /file-format/things/create to create things in file format Signed-off-by: Laurent Garnier <[email protected]>
I agree, that could be a good first usage of these new APIs.
And that would be also a solution to easily implement import of DSL things. I am in fact building globally something that will make easy going from unmanaged to managed things/items and vice versa. |
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]>
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]>
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]>
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]>
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]>
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]>
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]>
@florian-h05 @mherwege @jimtng : PR #4630 will allow to add a new tab that could be named "File format" in the item configuration page and the thing configuration page. Even if it should be also possible to let the user adjust item/thing by updating DSL code in this new tab, a first step probably not too much difficult to achieve in Main UI would be to provide the DSL code in this new tab as read-only. You could retrieve the DSL code just by calling the API /file-format/create using accept header text/vnd.openhab.dsl.item or text/vnd.openhab.dsl.thing, providing a FileFormatDTO object as request body that contains the current definition of item or thing. That would be fantastic to have that new tab. |
This is a lot to read, so I probably haven't gotten all the details, but I'd just like to add a couple of comments:
|
It was implemented with success in a relatively easy way! Thanks to XText. |
That's impressive. Still, I'd claim that one should differentiate between the "source DSL" and the generated DSL - there are different ways to express things, and the "decompiled" result might not resemble the original. Also, comments are probably lost due to JSONs limitation. That's one thing that actually annoys me with the current scheme where JSON is the "base format": The fact that JSON can't hold comments. YAML (as much as I dislike it in general) can, and when making widgets for example, it's very frustrating that all comments disappear when you save. It makes it difficult to keep track of things. Personally, I copy/paste the YAML back and forth to Notepad++ and doesn't actually "use" the JSON DB as the storage. That's how bad I think that is. |
The JSON can hold arbitrary elements though. So you can add a field like I'm neither advocating for this nor denigrating it. Just point it out as an option some people use. |
When we discuss about new format for configuration files, one wish would be to use a common (YAML) syntax in MainUI code tab and in the new file format. To make this dream a reality, a required initial step is to replace the conversion to/from YAML currently hardcoded in MainUI by new REST APIs in core in charge of these conversions. MainUI would then call these new APIs when needed without handling YAML conversion by itself. This would also allow to show easily another format (like our current DSL format) in the code tab just by passing the requested format to the API.
@openhab/webui-maintainers : I need your help to identify what are the APIs you need for that. I guess two APIs are necessary, one to convert to YAML and one to parse YAML.
When entering in the code tab, you need to build the YAML code from the current internal data, these data are potentially different from the thing stored in the registry because the user may have started to change configuration parameters for example. In #4569, I already added the API GET /things/{thingUID}/syntax/generate but it is based on the thing in the registry. So I believe I should add another similar API where you can pass as parameter thing data (ThingDTO) like in the API to create a new thing, and the returned output is the generated YAML code: POST /things/{thingUID}/syntax/generate
Then when leaving the code tab, you need an API to parse the YAML code potentially updated by the user and retrieve the corresponding thing data (ThingDTO). I could add a new API POST /things/{thingUID}/syntax/parse with a parameter in which you provide the YAML code and the returned output is corresponding thing data (ThingDTO).
@florian-h05 : please confirm it would be OK and sufficient for MainUI needs.
In the code tab, I can see that all channels are listed. Does it mean you support adding/removing of channels through this tab ? Or do you show them just for information ? Or just to let the user update a channel configuration parameter ?
Of course in the future config file, the user should not define all these channels while they can be deduced from the thing type. So it is a point to clarify.
The text was updated successfully, but these errors were encountered: