Add a YAML file provider for semantic tags

[lolodomo]

Files in folder conf/tags are loaded by this provider.

Related to #3619

Here is an example of file:

#
# Configuration file containing a list of custom semantic tags
#

version: 1

# For each semantic tag:
#  - uid and label are required
#  - description and synonyms are optional

tags:

  # Example of a new equipment
  - uid: Equipment_Curtain
    label: Curtain

  # Example of a new location
  - uid: Location_Indoor_Room_HomeCinemaRoom
    label: Home Cinema Room
    description: The room containing my home cinema system.
    synonyms:
      - Home theater room
      - TV room
      - Movie room

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

[J-N-K]

We should probably go one step back and think about the whole design of the YAML files. I'll create a RFC for that this afternoon as a GitHUB discussion.

[lolodomo]

I'll create a RFC for that this afternoon as a GitHUB discussion.

I don't find it, did you create it?

[J-N-K]

Sorry, I was a busy with real life issues. I'll try to write that this evening. With the concept I have in mind, the provider would not even need to handle anything except providing a DTO and registering itself with something similar to the ModelRepository.

[lolodomo]

Build failed because new dependencies with YAML libraries need to be added in each integration tests.

[lolodomo]

@J-N-K : I have now implemented your suggestion in #3666 . I would like your feedback.
I add comments on difficulties I encounter(ed) or changes compared to your initial suggestion.

[lolodomo]

It is the biggest problem I encountered.
@J-N-K : your suggestion was to use List<T> as parameter type. Makes sense. Unfortunately, in this case, I am not able to call these methods in processWatchEvent due to an error in Eclipse. Type argument is wrong and I don't know how to cast to the type expected by the compiler.
Advice would be appreciated.

[lolodomo]

@J-N-K : WDYT about that ?

[lolodomo]

A help is welcome to tell me what to add in itest.bndrun to make integration tests compile. Stuff related to YAML libraries has to be added.

[J-N-K]

The watch service listener code will not work this way. I'll create a PR to your repo that makes it work.

[lolodomo]

@J-N-K : any chance to have your help and review?

[J-N-K]

See my commit, I didn't test it, but it should work (at least the structure should now be ok).

[lolodomo]

See my commit, I didn't test it, but it should work (at least the structure should now be ok).

Thank you @J-N-K .
I am looking at your changes.
Having a map by model and not by file may be a good idea but the current code can't work as it is a mix between both.
When a file is deleted, we need to retrieve easily the removed objects. That was the idea with the map per file.
I will probably revert that or create a map of map.

[J-N-K]

You can also do it per-file, that doesn't matter. The important thing is that you need to correctly resolve the path that comes from the watch service.

[lolodomo]

You can also do it per-file, that doesn't matter. The important thing is that you need to correctly resolve the path that comes from the watch service.

Of course. I have not yet tested but will do in few minutes.
I am currently adding the map of map, I believe having a map per model 'like you proposed) to retrieve all objects of a model may be useful one day.

[lolodomo]

In fact, it is not exactly per model but rather per directory. I will return to a simple map per file.

[J-N-K]

It seem that e.g. the SemanticMetadataProvider does not implement a change listener for the SemanticTagRegistry, so tags added at a later stage are just ignored (until the item changes and processItem is called again). IMO the proper way would be to call allItemsChanged whenever the tag registry changes. This would also allow to completely remove the "wait for both providers" part on the SemanticTagRegistry. WDYT?

[lolodomo]

IMO the proper way would be to call allItemsChanged whenever the tag registry changes.

Yes and use the registry listener already available.

This would also allow to completely remove the "wait for both providers" part on the SemanticTagRegistry.

I am not sure about that...

[lolodomo]

IMO the proper way would be to call allItemsChanged whenever the tag registry changes.

Yes and use the registry listener already available.

Unfortunately, a class cannot implement RegistryChangeListener more than once with different arguments.
I can't add RegistryChangeListener in addition to ItemRegistryChangeListener.

Another option is to have a multiple reference to providers in SemanticsMetadataProvider and call allItemsChanged each time a provider is added or removed.
Edit: does not work because the new provider is not yet handled by the registry.

[J-N-K]

You can define a new interface for that SemanticTagRegistryChangeListener. We have that in other places, too.

[lolodomo]

You can define a new interface for that SemanticTagRegistryChangeListener. We have that in other places, too.

Yes, I am doing something that should work...

[lolodomo]

@J-N-K : I resolved the problem. Looks at the code please.

I will resume my tests.

[lolodomo]

@J-N-K : end of my tests, tests are successful. You can merge when you want.

[spacemanspiff2007]

Would it be possible to use the uid as a key in the yaml file:

So this

tags:
  # Example of a new equipment
  - uid: Equipment_Curtain
    label: Curtain
  # Example of a new location
  - uid: Location_Indoor_Room_HomeCinemaRoom
    label: Home Cinema Room
    description: The room containing my home cinema system.
    synonyms:
      - Home theater room
      - TV room
      - Movie room

Will become this:

tags:
  # Example of a new equipment
  Equipment_Curtain:
    label: Curtain
  # Example of a new location
  Location_Indoor_Room_HomeCinemaRoom:
    label: Home Cinema Room
    description: The room containing my home cinema system.
    synonyms:
      - Home theater room
      - TV room
      - Movie room

That way it would align with the suggestions I've made here

[lolodomo]

Would it be possible to use the uid as a key in the yaml file

I don't have a long history with YAML but I believe each element of a list must start with a -. So it would mean not using lists.
To match the suggested syntax, I have no idea how should look the java DTO for a correct parsing because you can't predict the object name.

[lolodomo]

I suggest you study this change after 4.1 is released, we don't have enough time now I believe.

[spacemanspiff2007]

I don't have a long history with YAML but I believe each element of a list must start with a -. So it would mean not using lists.

Correct, my suggestion is - instead of using a list - to use a mapping.

To match the suggested syntax, I have no idea how should look the java DTO for a correct parsing because you can't predict the object name.

You can't immediately de serialize the loaded file but have to implement some additional logic.
The most simple solution would be to just iterate over the mapping and pull down the object name into the object.
E.g. the second form gets transformed to the first form which then is used internally.

It's a trivial three to five liner - here in python

# validate the file input
file = validate_file_structure(file)

# pull down the uids
objects = []
for uid, obj_definition in file.tags.items():
    obj_def = obj_definition.copy()
    obj_def['uid]'] = uid
    objects.append(obj_def)

# now you can deserialize
deserialize_objects_from_dto(objects)

Using the uid as a key in a mapping is not much easier to write and makes the format much more compact, it also saves you some ambiguity checks (e.g. for duplicate UIDs) because duplicate key is an invalid yaml.

[lolodomo]

Please guys, nothing happened during 6 months and at 7 days before we freeze everything, you ask me "structural" changes ?
My free time yesterday was mainly spent to take into account good comments from @J-N-K and to run full tests again (and fix an issue).
I would like to spent my little free time during the next 6 days to something else, like for example helping to review PRs.

If the syntax is changed after 4.1 is released, this is not a problem. I don't think users will create thousands of custom tags and the change will be very easy to do, we will explain it to users.

[spacemanspiff2007]

Please guys, nothing happened during 6 months and at 7 days before we freeze everything, you ask me "structural" changes ?

I've only seen the provided example this morning, I believe it was not there before.

I've made detailed suggestions for a file format 2 weeks ago and made suggestions on design guidelines.
Like you I'd hate this yaml configuration push going stale because I think it's a great enhancement for the users.

If the syntax is changed after 4.1 is released, this is not a problem. I don't think users will create thousands of custom tags and the change will be very easy to do, we will explain it to users.

I'm fine with that but then we have to communicate it accordingly and beforehand.
Like:
"There is a possibility to create tags through this file format but this file format will change with the next releases".

[lolodomo]

Like:
"There is a possibility to create tags through this file format but this file format will change with the next releases".

"There is a possibility to create tags through this file format but this file format will change slightly with the next releases."
To not afraid users and because it is true ;)
Something to add in the release notes.

[J-N-K]

While I agree that the format suggested by @spacemanspiff2007 is probably better, it is also more difficult to implement. Jackson can de-serialize a Map OOTB, but

Equipment_Curtain:
  label: Curtain
Location_Indoor_Room_HomeCinemaRoom:
  label: Home Cinema Room
  description: The room containing my home cinema system.

is serialized into a Map which has Equipment_Curtain and Location_Indoor_Room_HomeCinemaRoom as key and ONLY the label and description elements in the value. The UID is therefore ONLY the key, not part of the value, which is wrong. It requires either duplicating the key as uid in the object (which is bad design), a custom de-serializer or another form of post-processing. My suggestion would be to merge as-is. Since we can read and write YAML files, we can convert while upgrading from version 1 to version 2.

[lolodomo]

My suggestion would be to merge as-is.

I like this suggestion :)

[spacemanspiff2007]

The UID is therefore ONLY the key, not part of the value, which is wrong.

Just to nitpick - it's not wrong it's just not de-serializable into java objects out of the box.
The amount of information is still the same, it's just ordered differently to make editing easier.
So the workflow would be load yaml-> post process -> to object.
But we really should have this discussion in the yaml file format issue.

For this PR I have no objections, just that the upcoming file change is communicated with the feature announcement.

[lolodomo]

It finally happens :) Thank you @J-N-K