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

[hue] Fix migration of API v1 legacy data to new v2 things #16714

Merged
merged 4 commits into from
May 9, 2024

Conversation

andrewfg
Copy link
Contributor

@andrewfg andrewfg commented May 4, 2024

Resolves #16711

When migrating a system from API v1 to v2, there could under some circumstances, be conflicts preventing a) the creation of new v2 things, and b) the migration thing attributes and linked items from the legacy v1 thing to the respective new v2 thing. This was due to ambiguous thing ids if there happened to be multiple v1 legacy things having identical thing ids and different thing type ids.

This PR resolves that issue by ensuring that:

  1. all new v2 things are created with a unique thing id, and
  2. thing attributes, and item channel linkages are copied from the correct v1 legacy thing

Signed-off-by: Andrew Fiddian-Green software@whitebear.ch

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
@andrewfg andrewfg added the bug An unexpected problem or unintended behavior of an add-on label May 4, 2024
@andrewfg andrewfg self-assigned this May 4, 2024
@andrewfg andrewfg requested a review from cweitkamp as a code owner May 4, 2024 10:22
@andrewfg andrewfg requested a review from a team May 4, 2024 10:23
@andrewfg
Copy link
Contributor Author

andrewfg commented May 4, 2024

@jlaur ping.

@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/philips-hue-clip-2-api-v2-discussion-thread/142111/473

@jlaur
Copy link
Contributor

jlaur commented May 4, 2024

@andrewfg - I did a quick test run by finding a sensor and a light with same numerical id:

  • GET http://hue.local/api/<key>/lights
  • GET http://hue.local/api/<key>/sensors

In my case I selected 38:

  • light:
    "38": {
      "name": "Snowdrop",
  • sensor:
    "38": {
      "name": "Hue ambient light sensor 6",

I then created a v1 bridge and these two devices as v1 things. But before testing a migration, I checked the console command (shortened to relevant output here):

openhab> hue hue:bridge-api2:home things
    // Device things
    Thing device 38 "Snowdrop" [resourceId="xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx"] // Color temperature light idV1:/lights/38

The sensor "Hue ambient light sensor 6" didn't show up. For the light, I suppose the expected result would be:

    // Device things
    Thing device xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx "Snowdrop" [resourceId="xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx"] // Color temperature light idV1:/lights/38

and that the sensor is also included?

@andrewfg
Copy link
Contributor Author

andrewfg commented May 4, 2024

@jlaur the 'Thing device 38 "Snowdrop"' is expected; it allows the v2 device 38 thing to be created and to take over the properties and linked items of the v1 [light] 38 thing. That is as intended.

However I am surprised that the console did not find the '38 Hue ambient light sensor' thing. => Is that correct?

EDIT: the console should have found that sensor thing but it's thing Id should not be 38 but rather a full v2 API resource id string. As the binding could not try to migrate the v1 38 sensor thing..

@jlaur
Copy link
Contributor

jlaur commented May 4, 2024

the 'Thing device 38 "Snowdrop"' is expected; it allows the v2 device 38 thing to be created and to take over the properties and linked items of the v1 [light] 38 thing. That is as intended.

Ah, got it. I created another v1 thing which also got a thing id like that.

However I am surprised that the console did not find the '38 Hue ambient light sensor' thing. => Is that correct?

Correct.

EDIT: the console should have found that sensor thing but it's thing Id should not be 38 but rather a full v2 API resource id string. As the binding could not try to migrate the v1 38 sensor thing..

Perhaps the generated thing ids should be "light-38" and "sensor-38" rather than just "38" to really solve this?

@andrewfg
Copy link
Contributor Author

andrewfg commented May 5, 2024

Perhaps the generated thing ids should be "light-38" and "sensor-38" rather than just "38" to really solve this?

I thought about that. It would avoid ambiguity between sensors and lights of the same name. And allow both sensors and lights to be migrated. However the issue is that we try to automatically migrate all the legacy items (and thus rules, sitemaps, etc) that are linked to 'hue:bridge:[light]:38:channelId' to 'hue:bridge-api2:device:38:channelId' -- And if we would actually change the thing id in that process, it would not be at all transparent. Indeed if we would change the id from 38 we might just as well use the new 20+ character resource id.


EDIT: rather than talking about (say) '38' maybe it helps to understand if I talk about a normal thing id. Lets say that a legacy system has one light and one switch in one room. There are basically two scenarios..

Scenario A the two legacy devices have unique names:

  • hue:bridge:[light]:kitchen-light migrates to hue:bridge-api2:device:kitchen-light
  • hue:bridge:[sensor]:kitchen-switch migrates to hue:bridge-api2:device:kitchen-switch

Scenario B the two legacy devices have identical names:

  • hue:bridge:[light]:kitchen migrates to hue:bridge-api2:device:kitchen
  • hue:bridge:[sensor]:kitchen migrates to hue:bridge-api2:device:kitchen => CONFLICT!!

EDIT 2:

For the avoidance of doubt, the issue is NOT primarily caused by two things having the Hue internal id '/sensors/38' resp. '/lights/38', but rather due to the fact that in OH those things might have been created as legacy things with the same thing id (38) albeit with different thing type id; and when those would be migrated to v2 then there is a conflict migrating due to the target thing id (38) and thing type id (device) being the same.

@andrewfg
Copy link
Contributor Author

andrewfg commented May 6, 2024

@jlaur the good news is that I tested it again on my system, and I confirm that it works as I had intended; the bad news is that I realised that there is a yet more complex edge case -- namely if the user would have one of each v1 type of light and one of each v1 type of sensor in their home then it would be theoretically possible for them to have created all of the v1 things below which all have unique ThingUIDs; however all of those unique things would be candidates to (try to) migrate their data to a v2 hue:device:abc:kitchen thing -- this could obviously be a major conflict situation.

hue:0000:xyz:kitchen
hue:0010:xyz:kitchen
hue:0100:xyz:kitchen
hue:0110:xyz:kitchen
hue:0200:xyz:kitchen
hue:0210:xyz:kitchen
hue:0220:xyz:kitchen
hue:0106:xyz:kitchen
hue:0107:xyz:kitchen
hue:0302:xyz:kitchen
hue:0820:xyz:kitchen
hue:0830:xyz:kitchen
hue:0840:xyz:kitchen
hue:0850:xyz:kitchen

Ideally the thing data migration v1 to v2 should use the same source and target thing id -- wherever possible -- since it is easier for the user to keep track of what is going on in their items, rules, and sitemaps etc. But in the case above there could be worst case 14 things with potentially the same id of which OH would consider 13 to be illegal.

I can think of a few possible ways forward..

  1. Only attempt a data migration for the first found thing with a given id. Difficulty is to define what is meant by 'first found' since this might be time variable. Note: this is somehow what I was attempting in this PR, but I now realise that even this 'solution' needs more work..
  2. Never attempt a data migration if the given thing id is used more than once.
  3. Use some other kind of id tagging scheme for v2 to be migrated device things -- for example as follows. Note: it is not ideal since a) the thing id does change in the data migration, b) after data migration to v2 the '0xxx_' prefix is a 'deprecated' past legacy that could confuse users in the future, c) it would require deeper explanation in readme etc. => @jlaur WDYT?
hue:0000:xyz:kitchen data migrates to hue:device:abc:0000_kitchen
hue:0010:xyz:kitchen data migrates to hue:device:abc:0010_kitchen
..
hue:0850:xyz:kitchen data migrates to hue:device:abc:0850_kitchen

@jlaur
Copy link
Contributor

jlaur commented May 6, 2024

@andrewfg - I think I do not yet have the full understanding. I'm now realizing I might already have started mixing up things here:

Perhaps the generated thing ids should be "light-38" and "sensor-38" rather than just "38" to really solve this?

I was considering the ids from the API, but actually they just happened to be auto-generated Thing IDs from the discovery. So in my case, the last part of the Thing UID was 38 for the light, but actually it could be anything if I had created that Thing manually. Hence your kitchen examples?

I would need to get back to this with more focus and understanding.

For now, my input would be:

  • Although it can certainly happen, and does happen, as we saw in the community forum case, this is still a corner-case scenario.
  • The migration itself is a one-time action.

Therefore, I wouldn't encourage you to invest too much in getting this totally right, although it probably itches a bit that it is currently flawed. To me, it doesn't make much difference if option 1 or 2 is chosen. Perhaps conflicts can be logged, in order to be handled manually by the user?

However, if you want to invest into getting this right, I can certainly assist, but would need some focus time in order to be helpful. I don't even remember the current "mechanism" for detecting when Things are migrated and how to migrate them.

A quick scratch in the surface. Mostly for my own train of thought, but please correct me where I'm wrong.

  • The v2 API provides id_v1, which is something like "/lights/38".
  • This is unique. There cannot be more than one light with id 38,
  • The light has a type, for example:
    "type": "Color temperature light",
  • This type is translated into a thing type. This is an openHAB concept:
    private @Nullable ThingTypeUID getThingTypeUID(FullHueObject hueObject) {
    String thingTypeId = TYPE_TO_ZIGBEE_ID_MAP
    .get(hueObject.getType().replaceAll(NORMALIZE_ID_REGEX, "_").toLowerCase());
    return thingTypeId != null ? new ThingTypeUID(BINDING_ID, thingTypeId) : null;
    }

    private static final Map<String, String> TYPE_TO_ZIGBEE_ID_MAP = Map.ofEntries(
    new SimpleEntry<>("on_off_light", "0000"),
    new SimpleEntry<>("on_off_plug_in_unit", "0010"),
    new SimpleEntry<>("dimmable_light", "0100"),
    new SimpleEntry<>("dimmable_plug_in_unit", "0110"),
    new SimpleEntry<>("color_light", "0200"),
    new SimpleEntry<>("extended_color_light", "0210"),
    new SimpleEntry<>("color_temperature_light", "0220"),
    new SimpleEntry<>("zllswitch", "0820"),
    new SimpleEntry<>("zgpswitch", "0830"),
    new SimpleEntry<>("clipgenericstatus", "0840"),
    new SimpleEntry<>("clipgenericflag", "0850"),
    new SimpleEntry<>("zllpresence", "0107"),
    new SimpleEntry<>("geofence", "geofencesensor"),
    new SimpleEntry<>("zlltemperature", "0302"),
    new SimpleEntry<>("zlllightlevel", "0106"));

So I think it's certainly possible to do the mapping and thus a reliable migration?

Now I get back to wondering why the Thing UID is significant at all in the migration process?

Sorry for the slow ride. 😄

@andrewfg
Copy link
Contributor Author

andrewfg commented May 7, 2024

@jlaur many thanks for your inputs. Yes it does 'itch'. But probably a bad fix would itch more.

why the Thing UID is significant at all in the migration process

You are probably right. The migration process basically does two things..

  1. Thing attribute migration: the new v2 thing is created with the same attributes as the existing v1 thing (e.g. description, location, etc.) -- whereby the thing id is one of those attributes -- but so long as description, location and linked items (see below) are migrated, it is perhaps not so important to retain the same thing id.
  2. Linked item migration: for all items that are linked to channels of the existing v1 thing, it links those items also to the corresponding channels on the new v2 thing

@andrewfg andrewfg added the work in progress A PR that is not yet ready to be merged label May 7, 2024
andrewfg added 2 commits May 7, 2024 10:25
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
@andrewfg andrewfg changed the title [hue] Prevent migration of things with ambiguous thing ids [hue] Fix data migration of legacy things May 7, 2024
@andrewfg andrewfg changed the title [hue] Fix data migration of legacy things [hue] Fix migration of API v1 legacy thing data to new v2 things May 7, 2024
@andrewfg andrewfg changed the title [hue] Fix migration of API v1 legacy thing data to new v2 things [hue] Fix migration of API v1 legacy data to new v2 things May 7, 2024
@andrewfg andrewfg removed the work in progress A PR that is not yet ready to be merged label May 7, 2024
@andrewfg
Copy link
Contributor Author

andrewfg commented May 7, 2024

@jlaur I think this is now fixed and ready for review.

Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM. Only one minor comment for consideration.

@jlaur
Copy link
Contributor

jlaur commented May 8, 2024

@andrewfg - new console test having added v1 sensor and light both with id 37 and adding "my label" in the Thing labels:

    Thing device 00000000-0000-0000-0000-000000000001 "Loftlampe my label" [resourceId="00000000-0000-0000-0000-000000000001"] // Color temperature light idV1:/lights/37
    Thing device 00000000-0000-0000-0000-000000000002 "Postkasse sensor my label" [resourceId="00000000-0000-0000-0000-000000000002"] // Hue motion sensor idV1:/sensors/37

The /sensors/38 previously mentioned did not appear probably because it's a v1 type 0106 "Hue ambient light sensor", so I'm guessing it doesn't exist as v2 resource. So my bad, my test was invalid.

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
@andrewfg andrewfg requested a review from jlaur May 9, 2024 08:29
@jlaur jlaur merged commit 1d3c52a into openhab:main May 9, 2024
5 checks passed
@jlaur jlaur added this to the 4.2 milestone May 9, 2024
PRosenb pushed a commit to PRosenb/openhab-addons that referenced this pull request May 22, 2024
…6714)

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
PRosenb pushed a commit to PRosenb/openhab-addons that referenced this pull request May 22, 2024
…6714)

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
psmedley pushed a commit to psmedley/openhab-addons that referenced this pull request Jun 15, 2024
…6714)

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
@andrewfg andrewfg deleted the hue-fix-#16711 branch August 25, 2024 16:05
pgfeller pushed a commit to pgfeller/openhab-addons that referenced this pull request Sep 29, 2024
…6714)

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Patrik Gfeller <patrik.gfeller@proton.me>
joni1993 pushed a commit to joni1993/openhab-addons that referenced this pull request Oct 15, 2024
…6714)

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
matchews pushed a commit to matchews/openhab-addons that referenced this pull request Oct 18, 2024
…6714)

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
cipianpascu pushed a commit to cipianpascu/openhab-addons that referenced this pull request Jan 2, 2025
…6714)

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior of an add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[hue] Potential thing UID conflict when mirroring legacy API v1 thing to API v2 Inbox thing
3 participants