Add semantic tag registry + REST API to manage user tags #3636

lolodomo · 2023-05-27T20:49:27Z

Related to #3619

New registry for semantic tags.
New default semantic tags provider for all built-in semantic tags.
New managed provider to add/remove/update user semantic tags.
Storage of user semantic tags in a JSON DB file.
New REST API to add/remove/update user tags in the semantic model.
New REST API to get a sub-tree of the semantic tags.

Tag label/description/synonyms are now in the registry, no more as part of the semantic class annotation.
Semantic tag class annotation are removed.

Semantic tag classes are now created at runtime.

Signed-off-by: Laurent Garnier [email protected]

lolodomo · 2023-05-27T20:53:34Z

It is already working well (managing custom tags through the REST API).
I have not yet fully implemented the update of a custom tag, it requires to update at runtime the TagInfo class annotation. I will have to search how to do that.

lolodomo · 2023-05-27T21:12:58Z

Build is failing but apparently with no link with my PR:

[INFO] --- sat-plugin:0.15.0:report (sat-all) @ org.openhab.core.automation.module.script ---
[INFO] Individual report appended to summary report.
[ERROR] Code Analysis Tool has found: 
 1 error(s)! 
 0 warning(s) 
 1 info(s)
[ERROR] org.openhab.core.automation.module.script.internal.ScriptEngineFactoryBundleTracker.java:[2]
Header line doesn't match pattern ^ \* Copyright \(c\) 2010-2023 Contributors to the openHAB project$

lolodomo · 2023-05-28T09:26:01Z

The build is now failing in automation integration tests:

[INFO] openHAB Core :: Integration Tests :: Automation Integration Tests FAILURE [ 39.314 s]
...
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  34:16 min (Wall Clock)
[INFO] Finished at: 2023-05-28T07:59:06Z
[INFO] ------------------------------------------------------------------------
[INFO] Static code analysis summary report is available in:
[INFO] file:/home/jenkins/jenkins-agent1/workspace/PR-openHAB-Core/target/summary_report.html
[ERROR] Failed to execute goal biz.aQute.bnd:bnd-testing-maven-plugin:6.4.0:testing (default) on project org.openhab.core.automation.integration.tests: 1 errors found -> [Help 1]

Is it something that could be linked to my changes ??

J-N-K · 2023-05-28T09:27:39Z

Since GitHUB CI succeeded, it's probably just an unstable test.

lolodomo · 2023-05-28T09:29:15Z

Since GitHUB CI succeeded, it's probably just an unstable test.

We will see at next compile when I add commits.

J-N-K

A conceptual question: Why don't we refactor the system tags to be provided by a tag provider and use the registry for everything?

...enhab.core.io.rest.core/src/main/java/org/openhab/core/io/rest/core/internal/tag/TagDTO.java

...s/org.openhab.core.semantics/src/main/java/org/openhab/core/semantics/CustomTagRegistry.java

....core.semantics/src/main/java/org/openhab/core/semantics/internal/CustomTagRegistryImpl.java

lolodomo · 2023-05-29T11:38:33Z

A conceptual question: Why don't we refactor the system tags to be provided by a tag provider and use the registry for everything?

I understand what you mean and that is a good remark.
If we add a system tag provider, how to be sure it will be loaded before custom tag providers?

lolodomo · 2023-05-29T11:46:02Z

That would mean we even do not need all the tag classes anymore?
This would require an analysis to determine what components would be impacted. Maybe HABot?

J-N-K · 2023-05-29T15:49:23Z

I don't think we can remove the tag classes. The parent/child relationship is implemented by class inheritance.

Regarding the YAML: I'll try to figure out how to to that. I'm also interested in that because in the long-term I would like to replace the XText files with YAML, which would also allow us simply copy & paste from UI code-tab to files (and back).

splatch

I been watching in which direction custom tags are going since #3519. Here I just reflect briefly my humble option that semantic tags been built for fixed purpose and they beg for ready markers, own start level and second thought. I do question the way in which they function right now. Below description is not opinion on this PR, but problem statement which I try to outline.

Because you mutate SemanticTag singleton instance you will face a lot of reliability issues. Depending on invocation order it will behave completely differently. More over there is currently no synchronization point which would deffer startup of code which register tags from code which intends to uses it. More over, since methods are visible to anyone it is possible to append tags at runtime. It is likely to see situations where execution of a rule with a blind SemanticsTag.add call will amend its internal state without invalidation of caches built elsewhere.

Two main notes:

TagInfo mapping which rely on annotation attributes bring no benefit to anyone. It is useless at runtime.
All TagInfo attributes could in fact become methods of Equipment/Point/Location types.
There is special meaning of TagInfo#id attribute, which is a leaking logic. Others have to preserve format of it, even if there is proper inheritance information (i.e. id=Equipment_AlarmSystem, reflect interface AlarmSystem extends Equipment, making id=AlarmSystem will ignore tag!).
You can't reliably validate information placed in annotations without building an annotation processor fired before any code is loaded (see Java Annotation Processing Tool).
Registration of tags which rely on SemanticsTag.add() and generated classes at runtime is an workaround for above point and attempts to escape static nature of SemanticTags, why not making a CustomEquipment/CustomLocation classes and use them instead?

If you keep going with changes made in #3519 please make sure that user documentation have big fat warning about risks it brings to them and how to properly use these across different places.

Custom tags will not be properly recognized by SemanticsMetadataProvider, if they are registered later. There is fair chance that they will work on random basis, depending on initialization order of elements which register tags at runtime.

J-N-K · 2023-05-29T19:08:41Z

@lolodomo 6cf06ff Looks quite easy.

splatch · 2023-05-29T20:17:39Z

6cf06ff Looks quite easy.

@J-N-K It does not cover item metadata, which is a separate concept but stored fairly close in jsondb and items files.

J-N-K · 2023-05-29T20:27:00Z

I'm aware of that, also links can't be provided at the moment. It was just a PoC for reading from YAML in a Provider. And it seems that reading YAML is as easy as two lines plus writing the DTO.

splatch · 2023-05-29T20:30:50Z

Agree & ack, it will cut by half amount of WTF moments new comers have when looking at UI definitions and file configurations.

lolodomo · 2023-05-29T20:37:07Z

I propose config files to be supported in a next PR.

lolodomo · 2023-05-29T23:06:48Z

All TagInfo attributes could in fact become methods of /Point/Location types.

Or label/description/synonyms could be a static member of each tag class?
This will make their updates easy without the need to update the class annotation.
WDYT?

splatch · 2023-05-29T23:23:29Z

Or label/description/synonyms could be a static member of each tag class?
This will make their updates easy without the need to update the class annotation.
WDYT?

Built-in tags can for sure rely on static values, user supplied tags can be initialized with data stored in JSON or any other format. You will need a CustomLocation, CustomPoint, CustomProperty for each case in order to keep instanceof operations. It might be a bit of work for SemanticsProvider. Not sure if existing semantics support nested conditions (i.e. "terrace is outside location"), even if it does, it does it based on declared parent and not compiled one.

lolodomo · 2023-05-29T23:26:25Z

In case we refactor to have the system tags in the registry (as suggested by @J-N-K ), the label/description/synonyms are probably no more needed on the tag class itself, they will be present in the registry.

This was not my intention to fully refactored all the semantic tags.

lolodomo · 2023-05-29T23:29:15Z

You will need a CustomLocation, CustomPoint, CustomProperty for each case in order to keep instanceof operations.

Sorry, I don't understand why.
You want to separate custom tags from system tags ?
My idea was to let users add a new custom tag as child of an existing system tag for example.

splatch · 2023-05-29T23:43:42Z

You want to separate custom tags from system tags ?
My idea was to let users add a new custom tag as child of an existing system tag for example.

Its up to you to decide how to approach this issue. I refereed to CustomLocation since "system" tags could be generated as static classes. However, if everything goes into registry then they could rely on single type. Frankly speaking, as long as there is clear interface for them to interact it does not matter how/where class is declared.

lolodomo · 2023-05-31T16:28:54Z

Having all tags in the registry is I believe possible. We could have a system tag provider that will fill the registry with all system default tags.
Then we should also move some static methods from SemanticTags to SemanticsService and use the registry.
Tag label, description and synonyms will then be only in the registry, no more in tag annotation. We will keep only the tag id in the annotation.
This will also require to update HABot to use the new methods in SemanticsService rather than the static methods from SemanticTags. Only the class SemanticsItemResolver is concerned in HABot.
This will not be a problem to be sure that the system tags are put in the registry before the managed user tags.
The REST API will then use only the registry and no more directly the tags.
I can try to do it ;)

lolodomo · 2023-06-05T17:02:06Z

I have now finished the refactoring of semantic tags to handle all of them in a new registry. Unit tests are now ok, integration tests also and I have done robust manual tests with the REST API.
There is only one point I am not fully satisfied, this is how to handle properly the adding of providers in the right order, meaning the default provider must be added first (before the managed provider).
@J-N-K : Would you like to restart a review ?

...ore.semantics/src/main/java/org/openhab/core/semantics/internal/SemanticTagRegistryImpl.java

lolodomo · 2023-06-05T17:05:32Z

This PR is of course a breaking API change but only HABot is impacted. I am now preparing the small adjustment required in HABot. As soon as this PR is merged, the HABot PR should also be merged.

Related to openhab#3619 New registry for semantic tags. New default semantic tags provider for all built-in semantic tags. New managed provider to add/remove/update user semantic tags. Storage of user semantic tags in a JSON DB file. New REST API to add/remove/update user tags in the semantic model. New REST API to get a sub-tree of the semantic tags. Tag label/description/synonyms are now in the registry, no more as part of the semantic class annotation. Signed-off-by: Laurent Garnier <[email protected]>

...ore.semantics/src/main/java/org/openhab/core/semantics/internal/SemanticTagRegistryImpl.java

...hab.core.semantics/src/main/java/org/openhab/core/semantics/model/equipment/AlarmSystem.java

Depends on openhab/openhab-core#3636 Signed-off-by: Laurent Garnier <[email protected]>

Signed-off-by: Laurent Garnier <[email protected]>

…in the right order Signed-off-by: Laurent Garnier <[email protected]>

lolodomo · 2023-06-07T11:10:01Z

@J-N-K : it is again ready for an additional review. Now, the tag classes are created at runtime.

lolodomo · 2023-06-07T16:31:11Z

Looking all remaining places where TagInfo is still used, I now believe that the annotation could be removed, we just need a method that would build the UID for a tag class considering the class hierarchy.

lolodomo · 2023-06-07T18:02:29Z

Looking all remaining places where TagInfo is still used, I now believe that the annotation could be removed, we just need a method that would build the UID for a tag class considering the class hierarchy.

It is done.

Unfortunately, the PR does not update (even if I do a force) ! I do not understand why.

lolodomo · 2023-06-07T18:15:49Z

Unfortunately, the PR does not update (even if I do a force) ! I do not understand why.

As the PR does not synchronize, I close it and I will open a new one.

splatch

I had a quick look on it, leaving some remarks in case if you are interested in them.

splatch · 2023-06-07T18:09:03Z

...st.core/src/main/java/org/openhab/core/io/rest/core/internal/tag/EnrichedSemanticTagDTO.java

+ * @author Jimmy Tanagra - initial contribution
+ * @author Laurent Garnier - Class renamed and members uid, description and editable added
+ */
+public class EnrichedSemanticTagDTO {


Small note on consistency - it doesn't seem to be much enriched compared to other enriched DTO types we have at REST layer, its just an regular representation which does not ship anything beyond SementicTag itself. Or I am wrong?

Enriched just because of the "editable" property.

....core.io.rest.core/src/main/java/org/openhab/core/io/rest/core/internal/tag/TagResource.java

splatch · 2023-06-07T18:14:29Z

....core.io.rest.core/src/main/java/org/openhab/core/io/rest/core/internal/tag/TagResource.java

+ .sorted((element1, element2) -> element2.compareTo(element1)).collect(Collectors.toList());
+ for (String id : uids) {
+ // ask whether the tag exists as a managed tag, so it can get updated, 405 otherwise
+ if (managedSemanticTagProvider.get(id) == null) {


Can this be simplified to a place where we just confirm given tag id is managed or not? I suppose we will not reach a place where parent tag might be managed and its children might not?

This is currently not possible but could become possible later when an unmanaged provider is added.

I am not yet sure if we should or not allow unmanaged tags to be added as childs to a managed tag and vice-versa. Because this will make loading of unmanaged and managed providers very difficult.

...re.model.script/src.moved/test/java/org/openhab/core/model/script/actions/SemanticsTest.java

bundles/org.openhab.core.semantics/src/main/java/org/openhab/core/semantics/Properties.java

splatch · 2023-06-07T18:23:02Z

...b.core.semantics/src/main/java/org/openhab/core/semantics/internal/SemanticsServiceImpl.java

+ .collect(Collectors.toList());
+ List<Class<? extends Tag>> tagList = new ArrayList<>();
+ tags.forEach(t -> {
+ Class<? extends Tag> tag = SemanticTags.getById(t.getUID());


I think the Tag classes are not serving any major purpose any more, you can simply rely on SemanticTag instances all over this place and related ones.

At least some of them are for compatibility reasons with HABot. That is exactly the case for method getByLabelOrSynonym
But I will remove the others not necessary (not used in core or HABot).

I removed some of the methods I previously added in this class.
But the method you pointed is required for HABot compatibility.

…ass (#1916) Depends on openhab/openhab-core#3636 Signed-off-by: Laurent Garnier <[email protected]>

lolodomo requested a review from a team as a code owner May 27, 2023 20:49

lolodomo marked this pull request as draft May 27, 2023 20:49

J-N-K added rebuild Triggers the Jenkins PR build and removed rebuild Triggers the Jenkins PR build labels May 28, 2023

lolodomo force-pushed the custom_tags branch 2 times, most recently from c4ef256 to f82c644 Compare May 29, 2023 07:55

J-N-K requested changes May 29, 2023

View reviewed changes

splatch approved these changes May 29, 2023

View reviewed changes

lolodomo force-pushed the custom_tags branch from 19a7db8 to c9a7054 Compare June 1, 2023 21:02

lolodomo changed the title ~~Add custom tag registry + REST API to manage custom tags~~ [WIP] Add semantic tag registry + REST API to manage semantic tags Jun 1, 2023

lolodomo force-pushed the custom_tags branch from c9a7054 to 044e545 Compare June 1, 2023 21:03

lolodomo force-pushed the custom_tags branch 7 times, most recently from 8023ca2 to 741d481 Compare June 5, 2023 16:54

lolodomo changed the title ~~[WIP] Add semantic tag registry + REST API to manage user tags~~ Add semantic tag registry + REST API to manage user tags Jun 5, 2023

lolodomo marked this pull request as ready for review June 5, 2023 16:56

lolodomo commented Jun 5, 2023

View reviewed changes

...ore.semantics/src/main/java/org/openhab/core/semantics/internal/SemanticTagRegistryImpl.java Outdated Show resolved Hide resolved

lolodomo force-pushed the custom_tags branch from 741d481 to f6670ea Compare June 5, 2023 17:30

J-N-K reviewed Jun 5, 2023

View reviewed changes

lolodomo added a commit to lolodomo/openhab-webui that referenced this pull request Jun 5, 2023

[HABot] Use SemanticsService to search for label/synonyms of a tag class

c5629b8

Depends on openhab/openhab-core#3636 Signed-off-by: Laurent Garnier <[email protected]>

lolodomo mentioned this pull request Jun 5, 2023

[HABot] Use SemanticsService to search for label/synonyms of a tag class openhab/openhab-webui#1916

Merged

lolodomo added 4 commits June 5, 2023 20:20

Review comment: override addProvider to load the default provider first

575785a

Signed-off-by: Laurent Garnier <[email protected]>

Review comment: semantic tag classes created at runtime

4a0a909

Signed-off-by: Laurent Garnier <[email protected]>

Add managed provider in tag registry constructor

25701dc

Signed-off-by: Laurent Garnier <[email protected]>

Sort managed tags by uid to be sure that tag classes will be created …

d4dbc58

…in the right order Signed-off-by: Laurent Garnier <[email protected]>

lolodomo closed this Jun 7, 2023

splatch reviewed Jun 7, 2023

View reviewed changes

lolodomo mentioned this pull request Jun 7, 2023

[rest] Add semantic tag registry + API to manage user tags #3646

Merged

kaikreuzer pushed a commit to openhab/openhab-webui that referenced this pull request Jun 17, 2023

[HABot] Use SemanticsService to search for label/synonyms of a tag cl…

81e12f5

…ass (#1916) Depends on openhab/openhab-core#3636 Signed-off-by: Laurent Garnier <[email protected]>

J-N-K mentioned this pull request Jun 20, 2023

RFC: YAML configuration #3666

Open

Add semantic tag registry + REST API to manage user tags #3636

Add semantic tag registry + REST API to manage user tags #3636

Conversation

lolodomo commented May 27, 2023 • edited Loading

lolodomo commented May 27, 2023

lolodomo commented May 27, 2023

lolodomo commented May 28, 2023

J-N-K commented May 28, 2023

lolodomo commented May 28, 2023

J-N-K left a comment

Choose a reason for hiding this comment

lolodomo commented May 29, 2023

lolodomo commented May 29, 2023

J-N-K commented May 29, 2023

splatch left a comment • edited Loading

Choose a reason for hiding this comment

J-N-K commented May 29, 2023

splatch commented May 29, 2023 • edited Loading

J-N-K commented May 29, 2023

splatch commented May 29, 2023

lolodomo commented May 29, 2023

lolodomo commented May 29, 2023

splatch commented May 29, 2023

lolodomo commented May 29, 2023

lolodomo commented May 29, 2023

splatch commented May 29, 2023

lolodomo commented May 31, 2023

lolodomo commented Jun 5, 2023

lolodomo commented Jun 5, 2023

lolodomo commented Jun 7, 2023

lolodomo commented Jun 7, 2023

lolodomo commented Jun 7, 2023

lolodomo commented Jun 7, 2023

splatch left a comment

Choose a reason for hiding this comment

splatch Jun 7, 2023

Choose a reason for hiding this comment

lolodomo Jun 7, 2023

Choose a reason for hiding this comment

splatch Jun 7, 2023

Choose a reason for hiding this comment

lolodomo Jun 7, 2023

Choose a reason for hiding this comment

splatch Jun 7, 2023

Choose a reason for hiding this comment

lolodomo Jun 7, 2023 • edited Loading

Choose a reason for hiding this comment

lolodomo Jun 7, 2023

Choose a reason for hiding this comment

lolodomo commented May 27, 2023 •

edited

Loading

splatch left a comment •

edited

Loading

splatch commented May 29, 2023 •

edited

Loading

lolodomo Jun 7, 2023 •

edited

Loading