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

[Automation] Paper UI (JSON) rules fail to load after ESH reintegration #674

Closed
5iver opened this issue Mar 21, 2019 · 27 comments
Closed

[Automation] Paper UI (JSON) rules fail to load after ESH reintegration #674

5iver opened this issue Mar 21, 2019 · 27 comments

Comments

@5iver
Copy link

5iver commented Mar 21, 2019

Before the ESH reintegration, Paper UI saved rule files with...

"class": "org.eclipse.smarthome.automation.dto.RuleDTO"

After the ESH reintegration (observed on S1557), the rules are saved with...

"class": "org.openhab.core.automation.dto.RuleDTO"

This causes the following error after OH is upgraded and the rule files are loaded...

2019-03-21 15:22:55.362 [ERROR] [org.eclipse.smarthome.storage.json.internal.JsonStorage] - Couldn't deserialize value 'org.eclipse.smarthome.storage.json.internal.StorageEntry@4aee9d1c'. Root cause is: org.eclipse.smarthome.automation.dto.RuleDTO cannot be found by org.openhab.core.automation_2.5.0.201903152356

After upgrading OH, users that have created rules in Paper UI will need to:

  1. Stop OH
  2. Using the openhab account, edit /userdata/jsondb/automation_rules.json
  3. Search/replace the class name
  4. Save the file
  5. Repeat with /userdata/jsondb/automation_rules_disabled.json
  6. Delete the files in /userdata/jsondb/backup directory
  7. Restart OH

It wouldn't help people with manual installs, but could this be added to an update script?

@davidgraeff
Copy link
Member

This was a breaking change, so we are already in the OH3 breaking changes phase? ^^

@kaikreuzer
Copy link
Member

but could this be added to an update script?

Yes, and this was also discussed already at the time (can't find a link now). This will have to be tackled by a migration script (we have other similar changes like the pids for the UIs etc.).

@5iver 5iver changed the title [Automaiton] Paper UI (JSON) rules fail to load after ESH reintegration [Automation] Paper UI (JSON) rules fail to load after ESH reintegration Apr 4, 2019
@5iver
Copy link
Author

5iver commented Apr 4, 2019

@kaikreuzer, do you know if these have already been documented somewhere? If not I'll get an issue together to start tracking them. I'm assuming the migration script would be required for 2.5 M2.

@itskeKs
Copy link

itskeKs commented Apr 9, 2019

@openhab-5iver just tried to do your suggestion. after starting openhab again it changes the class into „org.eclipse.smarthome.automation.Rule“ and still throws the error at me. also all the rules are gone again like after the update.

@5iver
Copy link
Author

5iver commented Apr 9, 2019

I just updated the steps I had documented... delete the backups.

@maggu2810
Copy link
Contributor

FYI for MapDB I use this modification that apply the migration as ESH decided to use DTOs and the migrate as we moved the package name from ESH to OHC:
maggu2810@be8b0e8

@kaikreuzer
Copy link
Member

I think we will need a similar mechanism for many further situations in future, when we switch away from the Eclipse namespace. I would hence not do a migration at runtime in the various places, but rather operate on the filesystem (we will also have to cover cfg files and similar).

@wborn & @BClark09, in openhab/openhab-distro#501, we also briefly discussed how to deal with migrating files through the update script, so I'd like your opinions here as well: We might be able to edit cfg and json (and other) files through scripts, but this might become complex, especially to have it working for Unix & Windows likewise. I wonder if we should use something like a Groovy script or maybe even a specific bundle that is started with lowest start level and which takes care of required migration of files. I am really not sure what would be the best option...

@BClark09
Copy link
Member

BClark09 commented Jul 9, 2019

If it has to happen on the update script, I can't think of any other way than adding a new [[POST]] command to the update.lst:

REPLACETXT;[Directory];[Text to Find];[Text to Replace]

This could be handled with a sed command or whatever the Powershell equivalent is.

But as you say, syncing this with Manual and Packaged installs, as well as Windows, Mac and Linux may be fairly tricky.

specific bundle that is started with lowest start level and which takes care of required migration of files.

I like this idea, openHAB runtime becomes responsible for its own filesystem and is a platform independent solution. You could use a flag to tell the bundle not to start if already run once after an install or update.

@maggu2810
Copy link
Contributor

Why not hooking into the Karaf startup and apply the migration using Java code?

  • you can use a much more comfortable solution that working with sed expressions
  • it works on every OS openHAB is running
  • it does not matter if an manual installation or a package manager based one is used
  • ...

@J-N-K
Copy link
Member

J-N-K commented Jul 9, 2019

Could this be used to upgrade things, too? A lot of breaking changes/additions which currently require to delete and re-add things could be made simpler with upgrading the database. The binding developer should then provide code that updates the things.

@maggu2810
Copy link
Contributor

There is currently no "standard" way to handle things changes in bindings.

I already handled it some time in bindings of my own by adding a version to my thing and migrate the things myself. But I assume openHAB should create its standard mechanism themself.

But IMHO this require that we stop using snapshots and use real versions (e.g. revision releases).
Or another clever mechanism how a binding developer could know about the different versions of its bindings.

I assume we would like to support updates for official releases and not for every snapshot, otherwise it could be very complicated if bindings changes thing types frequently on development.

@kaikreuzer
Copy link
Member

Why not hooking into the Karaf startup and apply the migration using Java code?

I think this what I meant with the option "maybe even a specific bundle that is started with lowest start level". Is there any special hook in Karaf that we could use and that would make it easy to add such a logic?

@J-N-K Wrt Thing migrations, I agree with @maggu2810 that this is a separate&complex topic that I do not see as a part of this issue here. I think if we could find a solution for eclipse-archived/smarthome#2555, a lot of potential issues wrt Thing migration would already be tackled without needing any special migration logic at all.

@maggu2810
Copy link
Contributor

IMHO we need first define how such an update should be handled at all.

  • Should the migration code be able to upgrade every possible storage implementation (currently mapdb, json, later perhaps also yaml, ...)?
  • Should we be able to migrate between storage implementation (mapdb => json) e.g. if someone is no longer used, maintained, ...?
  • Should the migration code know about all possible files that needs a migration or should there be hooks (e.g. using OSGi services)?
  • Should the migration code be one big base that contains all its need or do we require an OSGi context?
  • ...

The question about the "one big base" vs. "OSGi" would already dictate how the integration in the runtime needs to be done. Do we need the OSGi runtime container or can we simply start a jar file in front of the karaf container. If we need an OSGi container, we need to define the requirements. If we e.g. do not use the normal storage implementations that are available at runtime but want to ensure that the migration is executed in front of, we could require that the storages requires a migrationdone marker, ...

@bjoernbrings
Copy link
Contributor

@maggu2810 both solutions sound to me rather complex to implement for getting the milestone kicked off. Would there be a quickfix to get this done? WDYT?

@maggu2810
Copy link
Contributor

It depends on which files needs to be migrated.

For the storage I linked (IIRC) an example for the MapDB one.

@kaikreuzer
Copy link
Member

@maggu2810 For me the scope of this is very limited. The only thing I would want to address with it are breaking changed due to ESH->openHAB namespace refactoring, i.e. no "universal migration/compatibility framework".
It should help users to do some required steps, so that we don't have to ask them to do it manually (by mentioning them in the release notes).
As said above, I see this as a part of the openHAB distro update script and it could simply by done by some sed commands.
So the solution I'd like to discuss actually belongs to openhab-distro and not openhab-core. From this perspective, it is good enough to address json storages as the openHAB distro does not use any other (and as you already have a solution for yourself, I assume you are fine with that limitation).

In general, it is rather a "run once" logic, specific for finding org.eclipse.smarthome remains and changing them to org.openhab - if this has been performed, the logic can be disabled/removed as @BClark09 suggests.

So imho the only reason for doing it in Java instead of using sed is to be platform independent. And I wonder, whether it is a good or bad idea to have it started through Karaf, so I was wondering if you know any specific hooks that might be suitable for it. If not, we could also build a small class that gets executed before Karaf is started.

@maggu2810
Copy link
Contributor

if you know any specific hooks that might be suitable for it

I don't know that by heart. My approach would have been to read the scripts and then draw my conclusions.

@kaikreuzer
Copy link
Member

If it has to happen on the update script, I can't think of any other way than adding a new [[POST]] command to the update.lst:

@BClark09 Further contemplating about it, I actually think that this solution might fit best to the requirements I have listed above. Such a "REPLACETXT" command would be pretty generic and we could use the update.lst file for the specific infos on what needs to be changed.

syncing this with Manual and Packaged installs, as well as Windows, Mac and Linux may be fairly tricky.

The only places which we need to tackle are "userdata" and potentially "conf" - if we have the respective variables set in the update script, this should hopefully work.

Could you come up with a PR for openhab-distro adding such a command using sed for Unix? We could have a try with this as a straight-forward solution to the problem.

@wborn
Copy link
Member

wborn commented Jul 20, 2019

Here are my thoughts on the various migration options:

Generic Storage migration implementation

I think it will be a lot of work to properly add migration functionality for all Storage implementations to OH core. Definitely not something that can be easily accomplished in time for a 2.5.0 M2 release.

For developers using the framework it would also be helpful if they can easily plug-in their own migration logic for their own defined Storages. So developers should then be able to define what migrations should be done on a Storage before it becomes available. Maybe it's possible to define these migrations in a Storage independent way not using the actual objects (so it's possible to remove the classes). I.e. the migrations could be a set of operations working on the objects (stored by the Storage) in a representation similar to JsonObjects? It would then also make sense to keep track of what migrations have been done per Storage (similar to database patch levels).

JsonStorage migration

Based on @maggu2810's migration implementation I had a look if something similar can be done with Gson. For Gson I found gson-fire which adds Object pre and post processing to Gson. But from the first looks it doesn't seem to cover our use case where object classes change. Looks like the Gson TypeAdapter and TypeAdapterFactorys need the actual classes which no longer exist in our case. If the fix is made in the JsonStorage, the most simple fix/workaround is probably to just patch the content read from the inputFile in readDatabase.

Update script migration

Adding and using the REPLACETXT command is also a good way for now. It will definitely come in handy when all packages are changed from ESH to OH in OH3.

When this command just replaces all occurrences in a file with something else it's fine to implement it in .sh/.bat files.

As soon as it starts using all pattern matching functionality sed has, it's probably better to start using Groovy/Java RegExps using Patterns instead of sed. It might prevent a lot of issues. I don't think Windows has a sed command so we might need to package a binary for it or find some shell script that mimics it and hope it doesn't give all sorts of issues.

An issue with using an update script for doing the migration is that it won't be automatically executed when launching openHAB from Eclipse.

@kaikreuzer
Copy link
Member

I don't think Windows has a sed command

I'd hope that a simple search&replace should do, if we are only talking about namespace changes - and that should be feasible on Windows as well. But we might lack the expertise, so this might be one reason to go for a small Java executable instead.

An issue with using an update script for doing the migration is that it won't be automatically executed when launching openHAB from Eclipse.

I think that's absolutely fine, there is no need for migration within the IDE use. I think the update script is the perfect place for it.

@bjoernbrings
Copy link
Contributor

Short google leads me to this solution from stackoverflow:
(Get-Content c:\temp\test.txt).replace('[MYID]', 'MyValue') | Set-Content c:\temp\test.txt
So it should be feasible on Windows as well

@kaikreuzer
Copy link
Member

Fixed by openhab/openhab-distro#946.

@5iver
Copy link
Author

5iver commented Aug 2, 2019

While I appreciate the efforts made, I respectfully do not think that this issue has been fully resolved, since not everyone will use the update script. There also has not been any documentation put together yet to educate users about the issue and proposed fix, or a way for them to resolve it other than using the update script when updating OH. As I mentioned in the PR...

The OOTB backup and update scripts are great templates, but they don't fit everyone's use cases. For example, I use a custom script to backup and update, and I know there are others who do the same... especially the people who have been using their own scripts since before the OOTB scripts existed.

@wborn also mentioned that the openHAB Docker containers do not use the update script.

I haven't found the time to read through the script to understand it's logic or to test it. What happens if a user updates OH without using the update script, and then the user runs the update script? Does it just bail out, or would the fix for the rule files still be applied? What other modifications would be performed? Can it be run in a Docker container? It would be great if we could tell people who don't use the update script and experience this issue to just run the update script to fix the problem.

@kaikreuzer
Copy link
Member

@openhab-5iver I think I answered this here. The update script (or maybe rather the processing of the update.lst file) is a mandatory step for the upgrade, hence no user needs to know anything about the technical details. If some official upgrade isn't using it yet, this needs to be fixed (and I think @wborn is already looking at Docker).

@wborn
Copy link
Member

wborn commented Aug 3, 2019

Yes I'm looking into using the update script with Docker. The openHAB synology, qnap and snap packages probably also currently don't use the update script.

Looks like the update script in openhab-distro is meant for updating manual installations so it needs some modifications when used with other installation packages. See also openhab/openhab-linuxpkg#148.

@wborn
Copy link
Member

wborn commented Aug 3, 2019

With openhab/openhab-docker#239 the openHAB Docker images will also use the common update files. That way the rules will also be patched by the new REPLACE command (openhab/openhab-distro#946).

@kaikreuzer
Copy link
Member

Excellent, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants