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

Improved: getConfigParam can't be configured at runtime (OFBIZ-12815) #634

Conversation

thahn27
Copy link

@thahn27 thahn27 commented May 5, 2023

In for example InterfaceConfig there are several methods which lead to UtilProperties instead of EntityUtilProperties and therefore cannot be configured during runtime.

Improved: In for example InterfaceConfig there are several methods which lead to UtilProperties instead of EntityUtilProperties and therefore cannot be configured during runtime. All of these methods don't access the database at all and just reference the UtilProperties methods. I rewrote the methods in EntityUtilProperties so first the database gets checked and if no entry in the database is found the fallback to UtilProperties happens. I left in all the current used methods and just placed my rewritten methods above the current used ones.

In for example InterfaceConfig there are several methods which lead to
UtilProperties instead of EntityUtilProperties and therefore cannot be
configured during runtime.
@sonarqubecloud
Copy link

sonarqubecloud bot commented May 5, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

return defaultValue;
}
try {
if (clazz.componentType() == Integer.class) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hello, since it is for trunk and java 17 is available, what do you think about using switch pattern matching (and Guarded Pattern)
ref : https://openjdk.org/jeps/406
Thanks and regards,
Gil

Copy link
Contributor

Choose a reason for hiding this comment

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

Mmm, i got carried away discovering those new switch syntax :), simple switch are sufficient here, sorry.

Copy link
Author

@thahn27 thahn27 May 8, 2023

Choose a reason for hiding this comment

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

Hey Gil,

i closed this PR and opened another one because the commit description was wrong. The new PR: #635
Also i will adjust the existing code with a switch in an upcoming commit in the new PR.

Thanks and regards,
Tobias

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks to you Tobias :)

Copy link
Author

Choose a reason for hiding this comment

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

Hey @gilPts ,

i tried to implement the switch statement instead of the if statements and i can't really make it work. The pattern matching documentation in the ref you provided works with objects which have been instantiated before. Maybe stick to the if statements?
Maybe you could provide an example or explain how you would implement such switch statement.

Thanks and regards,
Tobias

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes pattern matching is not applicable here, hence my second comment, sorry for the misunderstanding. If in those cases are fine, since we are operating on Class object test...
Thanks

@thahn27 thahn27 closed this May 8, 2023
@thahn27 thahn27 deleted the OFBIZ-12815_getConfigParam_in_InterfaceConfig_cannot_be_configured_during_runtime branch May 8, 2023 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants