search

openhab / openhab-addons

Add-ons for openHAB
https://www.openhab.org/
Eclipse Public License 2.0
1.86k stars 3.56k forks source link

Removal of dependency on 'org.apache.commons.*' #7722

Open cweitkamp opened 4 years ago

cweitkamp commented 4 years ago

In https://github.com/openhab/openhab-core/pull/1433 and https://github.com/openhab/openhab-core/pull/1441 (and more) we removed dependency on org.apache.commons.* libraries. They will be removed from OH3 Core API and thus not available anymore for OH add-ons too. Most of the methods which are used by several bindings are convenience helper methods and can be replaced easily by basic Java methods. Other methods are not yet available in Java 8 but in Java 11 and have to be replace later. It would be a good start to remove as much as we can before.

org.apache.commons.net is an exception be because is is used by org.openhab.core.serial and thus it will be okay for add-ons too.

lolodomo commented 4 years ago

I already created 12 PRs to eliminate calls to IOUtils.closeQuietly. As a first feedback in one of these PRs, @J-N-K said we should not close streams silently. So in additional to suppress the call to IOUtils.closeQuietly, we need to add a log. I will update the already existing PRs.

lolodomo commented 4 years ago

I already created 12 PRs to eliminate calls to IOUtils.closeQuietly. As a first feedback in one of these PRs, @J-N-K said we should not close streams silently. So in additional to suppress the call to IOUtils.closeQuietly, we need to add a log. I will update the already existing PRs.

Is it oj for all mainteners that we should not close stuff (streams) silently ? 3 of my PRs include a log in case of error and are ready for a review/merge. 9 of my PRs are now tagged with "WIP" waiting to know if this is the way to go or not.

cweitkamp commented 4 years ago

Imo it should be okay to log the exception even if I do not really expect it to happen too often. But we should check two other things in advance:

  1. Do we really need to close it? Or does the source of the stream takes care of it on its own (e.g. it is not necessary to close an InputStream from the Socket#getInputStream() method)
  2. Can it maybe handled even better by using a try-with-resources statement
lolodomo commented 4 years ago

https://docs.oracle.com/javase/8/docs/api/java/net/Socket.html#close-- Closing this socket will also close the socket's InputStream and OutputStream.

Ok, you're right, this is useless for socket. We have to check if it is required or not for serial port.

lolodomo commented 4 years ago

For serial port, it could depends on the framework implementation. gnu.io.SerialPort does not seem to close automatically the in/out streams. https://www.docjava.com/book/cgij/jdoc/gnu/io/CommPort.html#close()

lolodomo commented 4 years ago

In the Sonos binding, StringEscapeUtils.unescapeXml and StringEscapeUtils.escapeXml are used. What alternative do you propose ?

cweitkamp commented 4 years ago

That seems to be a hard one. I am not sure if there is a Java native equivalent available. At least not for Java 8. IIRC Java 13 added a method which does this: [String#translateEscapes](https://docs.oracle.com/en/java/javase/13/docs/api/java.base/java/lang/String.html#translateEscapes()).

Even the methods from commons-lang have been deprecated. As of 3.6, it is recommended to use commons-text StringEscapeUtils instead.

That could be one option. Add commons-text dependency for this bundle. Otherwise we could add our own implementation in a util class (XMLUtils). For OH2.5.x inside the binding. For OH3 in org.openhab.core.util or in org.openhab.core.io.net.http.

lolodomo commented 4 years ago

That seems to be a hard one. I am not sure if there is a Java native equivalent available. At least not for Java 8. IIRC Java 13 added a method which does this: [String#translateEscapes](https://docs.oracle.com/en/java/javase/13/docs/api/java.base/java/lang/String.html#translateEscapes()).

Even the methods from commons-lang have been deprecated. As of 3.6, it is recommended to use commons-text StringEscapeUtils instead.

That could be one option. Add commons-text dependency for this bundle. Otherwise we could add our own implementation in a util class (XMLUtils). For OH2.5.x inside the binding. For OH3 in org.openhab.core.util or in org.openhab.core.io.net.http.

We have 6 bindings using org.apache.commons.lang.StringEscapeUtils:

  1. amazonechocontrol
  2. bsblan
  3. bosesoundtouch
  4. homematic
  5. sonos
  6. wemo
lolodomo commented 4 years ago

To be honest, I have a big doubt this is really doable to remove ALL these dependencies. Too many are used in too many bindings.

fwolter commented 4 years ago

I filed a PR to remove Apache Commons from the Coding Guidelines default libraries: openhab/openhab-docs#1229

Hilbrand commented 3 years ago

@yfre please stop unpinning issues.

mherwege commented 3 years ago

That seems to be a hard one. I am not sure if there is a Java native equivalent available. At least not for Java 8. IIRC Java 13 added a method which does this: [String#translateEscapes](https://docs.oracle.com/en/java/javase/13/docs/api/java.base/java/lang/String.html#translateEscapes()). Even the methods from commons-lang have been deprecated. As of 3.6, it is recommended to use commons-text StringEscapeUtils instead. That could be one option. Add commons-text dependency for this bundle. Otherwise we could add our own implementation in a util class (XMLUtils). For OH2.5.x inside the binding. For OH3 in org.openhab.core.util or in org.openhab.core.io.net.http.

We have 6 bindings using org.apache.commons.lang.StringEscapeUtils:

  1. amazonechocontrol
  2. bsblan
  3. bosesoundtouch
  4. homematic
  5. sonos
  6. wemo

You can add the upnpcontrol binding to this list.

@cweitkamp @lolodomo Would unbescape be a possible alternative dependency to cope with the functionality of StringEscapeUtils? See here. The jar is 169k and I don't see external dependencies for it.

cweitkamp commented 3 years ago

Short feedback - with the latest code we still have some occurrences (463 imports) of org.apache.commons left:

$ grep -iR "import org.apache.commons.io" | wc -l
24
$ grep -iR "import org.apache.commons.lang" | wc -l
386
$ grep -iR "import org.apache.commons" | grep -v "commons.io" | grep -v "commons.lang" | wc -l
53

I do not think it will be possible to eliminate all of them until code freeze for OH3. Thus I would suggest to postpone this to first OH3 milestone. For some bundles like o.a.c.l.StringEscapeUtils, o.a.c.mail or o.a.c.net and others we do not have an appropriate replacement. Those dependencies can eventually be added to the bindings POM / feature directly - if not already done. This would pave the way for cleaning leftovers from OHC.

mherwege commented 3 years ago

@cweitkamp Maybe it got lost in the history (https://github.com/openhab/openhab-addons/issues/7722#issuecomment-679997251), but did you check unbescape as an alternative for StringEscapeUtils?

cweitkamp commented 3 years ago

@mherwege Do you know if the library is still supported? Latest release is nearly three years old.

mherwege commented 3 years ago

@mherwege Do you know if the library is still supported? Latest release is nearly three years old.

No, I don’t I found it when looking for an alternative, and to me, it looked like fitting the bill. I didn’t investigate much further.

cweitkamp commented 3 years ago

Alright. I do not see a strong argument against it. We can give it a try and include it - temporarily - as external dependency for the above mentioned bindings. Maybe write a short unit test to compare existing StringEscapeUtils against the new ones to avoid major breaking changes.

wborn commented 3 years ago

Short feedback - with the latest code we still have some occurrences (463 imports) of org.apache.commons left:

Only 159 occurrences left now. :slightly_smiling_face: 

$ grep -iR "import org.apache.commons.io" | wc -l
21
$ grep -iR "import org.apache.commons.lang" | wc -l
96
$ grep -iR "import org.apache.commons" | grep -v "commons.io" | grep -v "commons.lang" | wc -l
42

Note that all imports were updated in https://github.com/openhab/openhab-addons/pull/10314 to use org.apache.commons.lang3. The commons-lang3 dependency is also a required dependency for Swagger. So we currently have to ship this dependency anyways and adding another dependency just for getting rid of StringEscapeUtils would add more libraries to our distribution instead of less. :wink:

In commons-lang3 however StringEscapeUtils is deprecated as it has moved to commons-text.

For now I think it is best to continue reducing the use of org.apache.commons.* and eventually either provide replacements for missing functionality in openhab-core/add-ons or explicitly add dependencies to libraries in some add-on POMs and remove the commons libraries from the default compile dependencies to prevent them from being used.

openhab-bot commented 3 years ago

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

https://community.openhab.org/t/new-oh3-binding-midea-air-conditioning-lan/116613/23

openhab-bot commented 3 years ago

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

https://community.openhab.org/t/new-service-sma-semp-protocol-addons-io/59852/30

openhab-bot commented 2 years ago

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

https://community.openhab.org/t/plex-binding-for-oh3-will-it-be-ported/111487/138

Skinah commented 2 years ago

Removed another instance from systeminfo binding in this PR https://github.com/openhab/openhab-addons/pull/11322

Skinah commented 2 years ago

Another PR waiting by someone else that has removed some usages from the Astro binding https://github.com/openhab/openhab-addons/pull/11327

openhab-bot commented 2 years ago

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

https://community.openhab.org/t/ecma-script-ioutils-toinputstream/130338/1

openhab-bot commented 2 years ago

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

https://community.openhab.org/t/karaf-features-runtime-complaining-about-missing-import-whilst-is-has-been-included-at-compile-time/135832/5

openhab-bot commented 1 year ago

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

https://community.openhab.org/t/espmilighthub-new-binding-for-milight-limitlessled-and-easybulb/39720/545

lsiepel commented 1 year ago

I still see some bindings depending on org.apache.commons.* If needed i can try create some PR's to get those fixed. But before i spend time on it, How usefull is it? Are there any core util methods available and exposed to the addons that can be used? Most of the bindings use random string utils and html/xml escape helpers. So i hope not every binding has to implement its own string utility class?

FordPrfkt commented 1 year ago

I also was wondering about the reason for banning the use of org.apache.commons. ? Is there a license issue or something ? I am using org.apache.commons.net.ftp. in my binding and the build gives warnings about it, altough its use is still allowed. I think the checker should be changed to not warn about it but my Maven-Fu is weak so i am not sure where to look.

wborn commented 1 year ago

The reason is explained in the first comment @FordPrfkt .

lsiepel commented 1 year ago

Must agree to @FordPrfkt that the reason is not very clear, what problem is actually solved? Assuming we are allready beyond that point. I still have a questions regarding the utility classes. Many bindings use the same methods from org.apache.commons. like string escape /random etc. So it is expected that every binding implements its own methods for those cases? I can see bugs are going to be introduced here. Would be nice if some (most used) methods are exposed from core ?

Skinah commented 1 year ago

The reason is that in Java 11 and newer you do not need the Helper libraries as Java x has had its own methods included. We want to reduce the clutter, security issues, streamline, smaller size and better compatability that comes from removing external libs and using the latest LTS Java built in methods. It is easier to ensure we have the latest most secure code if we update Java and not have to worry about 80 other additional libraries which may not get the attention of regular updates and testing anymore as everyone has moved onto using Java 17 or higher.

wborn commented 1 year ago

Yes that is the lengthy version of the reasons. 👍

If you know how to write code without using these redundant libs, you can also use that expertise when contributing to other Java projects that do not want to introduce such library dependencies. It also goes the other way for people who want to contribute to openHAB but never used those libs.

FordPrfkt commented 1 year ago

OK, that totally makes sense, thanks for explaining it!

lsiepel commented 1 year ago

These bindings have a remaining import package for org.apache.commons i'll try to come up with some fixes, but certainly do not have the time to fix all of them.

lsiepel commented 1 year ago

@openhab/add-ons-maintainers As I'm almost half way, i like to get some reviews, merges or feedback before i proceed. It takes less time than i thought, so might do the others too.

edit: Sorry for all the PR's, but once i got started i could not help it and continued.

edit2: Finished for now. If the subnetutils fix is ok, I’ll apply them to the remaining ones. So maybe it is a good moment to look after the others.

I hope we can now 'blacklist' all the org.apache libs and only whitelist the ones below. @wborn mentioned .net earlier, but i would like to propose to add the others too, as they are more than just a simple convenience helper. If you look at the unscapehtml4 method for instance, it is complex, large and perfectly maintained outside of openHAB. Bringing that into openHAB would be introducing unnecessary maintenance if you ask me.

Maybe add a proxy utility class in core, addons can access those util classes and it re-routes to the apache libs. In that way it is easy to prevent these lib usage in addons and if anyone is ever feeling to imnplement them it can be done without any change in addons repo.

lsiepel commented 1 year ago

@hypetsch i could also fix the unescapeHtml4 for the bsblan binding, because it only unescapes units. I don't have such a device and the openapi documentation lacks details about possible units. Could you be of any help telling me the possible units? Is it only celcius and fahrenheit or are there more units?

hypetsch commented 1 year ago

I don't have a device with the latest firmware either, but from a quick look in the repo it seems to me that - at least in the latest version - the units depend on the localization and should for e.g. German be the "defines" starting with UNIT_ specified in https://github.com/fredlcore/BSB-LAN/blob/master/BSB_LAN/localization/LANG_DE.h

jlaur commented 1 week ago

Only a few left:

$ grep -R "org.apache.commons.lang3.StringUtils"
bundles/org.openhab.binding.sensibo/src/main/java/org/openhab/binding/sensibo/internal/model/SensiboModel.java:import org.apache.commons.lang3.StringUtils;
bundles/org.openhab.binding.sensibo/src/main/java/org/openhab/binding/sensibo/internal/model/SensiboSky.java:import org.apache.commons.lang3.StringUtils;
bundles/org.openhab.io.neeo/src/main/java/org/openhab/io/neeo/internal/TokenSearch.java:import org.apache.commons.lang3.StringUtils;
bundles/org.openhab.binding.fineoffsetweatherstation/src/main/java/org/openhab/binding/fineoffsetweatherstation/internal/domain/DebugDetails.java:import org.apache.commons.lang3.StringUtils;
$ grep -R "org.apache.commons.lang3"
bundles/org.openhab.binding.sensibo/src/main/java/org/openhab/binding/sensibo/internal/model/SensiboModel.java:import org.apache.commons.lang3.StringUtils;
bundles/org.openhab.binding.sensibo/src/main/java/org/openhab/binding/sensibo/internal/model/SensiboSky.java:import org.apache.commons.lang3.StringUtils;
bundles/org.openhab.io.neeo/src/main/java/org/openhab/io/neeo/internal/TokenSearch.java:import org.apache.commons.lang3.StringUtils;
bundles/org.openhab.persistence.mongodb/src/test/java/org/openhab/persistence/mongodb/internal/VerificationHelper.java:import org.apache.commons.lang3.tuple.Pair;
bundles/org.openhab.binding.fineoffsetweatherstation/src/main/java/org/openhab/binding/fineoffsetweatherstation/internal/domain/DebugDetails.java:import org.apache.commons.lang3.StringUtils;
itests/org.openhab.binding.max.tests/itest.bndrun:      org.apache.commons.lang3;version='[3.14.0,3.14.1)',\
lsiepel commented 1 week ago

Any suggestions on how to fix this for the mongodb? It depends on this Pair type and there is no drop in replacement. Without refactoring all the code in VerificationHelper i would end up making some kind of apache clone.

jlaur commented 1 week ago

How about something like:

public record Pair<L, R>(L left, R right) {}

?

The refactoring risk should be low as it is only used in a test class.

lsiepel commented 1 week ago

Thanks for the sugggestion. Just created a PR to get this done.