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

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

Open
cweitkamp opened this issue May 21, 2020 · 41 comments
Open

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

cweitkamp opened this issue May 21, 2020 · 41 comments

Comments

@cweitkamp
Copy link
Contributor

cweitkamp commented May 21, 2020

In openhab/openhab-core#1433 and openhab/openhab-core#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.

@wborn wborn pinned this issue May 21, 2020
lolodomo added a commit to lolodomo/openhab-addons that referenced this issue May 22, 2020
Relative to openhab#7722

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
lolodomo added a commit to lolodomo/openhab-addons that referenced this issue May 22, 2020
Relative to openhab#7722

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
lolodomo added a commit to lolodomo/openhab-addons that referenced this issue May 22, 2020
Relative to openhab#7722

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
lolodomo added a commit to lolodomo/openhab-addons that referenced this issue May 22, 2020
Relative to openhab#7722

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
lolodomo added a commit to lolodomo/openhab-addons that referenced this issue May 22, 2020
Relative to openhab#7722

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
lolodomo added a commit to lolodomo/openhab-addons that referenced this issue May 22, 2020
Relative to openhab#7722

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
lolodomo added a commit to lolodomo/openhab-addons that referenced this issue May 22, 2020
Relative to openhab#7722

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
lolodomo added a commit to lolodomo/openhab-addons that referenced this issue May 22, 2020
Relative to openhab#7722

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
lolodomo added a commit to lolodomo/openhab-addons that referenced this issue May 22, 2020
Relative to openhab#7722

Signed-off-by: Laurent Garnier <lg.hc@free.fr
lolodomo added a commit to lolodomo/openhab-addons that referenced this issue May 22, 2020
Relative to openhab#7722

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
lolodomo added a commit to lolodomo/openhab-addons that referenced this issue May 22, 2020
Relative to openhab#7722

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
lolodomo added a commit to lolodomo/openhab-addons that referenced this issue May 22, 2020
Relative to openhab#7722

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
lolodomo added a commit to lolodomo/openhab-addons that referenced this issue May 22, 2020
Relative to openhab#7722

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@FordPrfkt
Copy link

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
Copy link
Member

wborn commented Jan 12, 2023

The reason is explained in the first comment @FordPrfkt .

@lsiepel
Copy link
Contributor

lsiepel commented Jan 12, 2023

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
Copy link
Contributor

Skinah commented Jan 13, 2023

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
Copy link
Member

wborn commented Jan 13, 2023

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
Copy link

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

@lsiepel
Copy link
Contributor

lsiepel commented Feb 15, 2023

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
Copy link
Contributor

lsiepel commented Feb 17, 2023

@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.

  • StringEscapeUtils.unescapeHtml4 (i think this can be fixed here)
  • org.apache.commons.net.ftp
  • org.apache.commons.net.telnet
  • org.apache.commons.net.ntp
  • org.apache.commons.io.input
  • org.apache.commons.mail

@lsiepel
Copy link
Contributor

lsiepel commented Feb 27, 2023

@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
Copy link
Contributor

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
Copy link
Contributor

jlaur commented Jun 22, 2024

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
Copy link
Contributor

lsiepel commented Jun 23, 2024

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
Copy link
Contributor

jlaur commented Jun 23, 2024

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
Copy link
Contributor

lsiepel commented Jun 23, 2024

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

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

Successfully merging a pull request may close this issue.