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

[PAYARA-3909] - Can't Use Any of the Variable Types in JDBC Connection Pool Settings #4075

Merged
merged 10 commits into from Jul 18, 2019

Conversation

@cubastanley
Copy link
Contributor

commented Jul 5, 2019

It is desired that variable types can be used in place of set values in JDBC connection pool settings (i.e. set maxPoolSize = ${MAX_POOL_SIZE_PROPERTY}) - this was previously not possible due to the validation process of JDBC connection pool properties (results in a NumberFormatException trying to parse an incorrect format string to an Integer) - the changes in this PR get around that

@cubastanley cubastanley requested review from Cousjava, MeroRai and AlanRoth Jul 5, 2019

@cubastanley

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2019

Jenkins test please

@AlanRoth
Copy link
Contributor

left a comment

byootiful

@cubastanley

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2019

Jenkins test please

*
* @return possible object is
* {@link String }
*/
@Attribute (defaultValue="32")
@Min(value=1)
@Max(value=Integer.MAX_VALUE)
String getMaxPoolSize();

This comment has been minimized.

Copy link
@svendiedrichsen

svendiedrichsen Jul 5, 2019

Contributor

Isn't removing the min/max value begging for illegal values to be configured? Shouldn't they be at least checked for in code somewhere? Or rather keep them and extend validation to allow for variables whose value may be validated as well.

This comment has been minimized.

Copy link
@cubastanley

cubastanley Jul 8, 2019

Author Contributor

I agree it is a rather risky move, although as far as I'm aware these annotations will always cause a validation exception in the event that a user tries to input a string that cannot be parsed to an Integer (as would be the case with a system variable name) I've altered the basic check in the implementation of JdbcConnectionPool so as to prevent the value from being below 1 but I do agree that it isn't the safest situation

This comment has been minimized.

Copy link
@jbee

jbee Jul 15, 2019

Contributor

I think @svendiedrichsen has a good point here. Surely extending the mechanism isn't part of the task as originally requested but if we don't include this or make it a precondition of this PR we introduce room for error that might never be closed again even when we would like to see this happen in the near future.

@svendiedrichsen

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2019

How about supporting the existing variable mechanism and its different types here: https://docs.payara.fish/documentation/payara-server/server-configuration/var-substitution/

Would thus automatically support MicroProfile config.

@cubastanley

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

Jenkins test please


//By this point it should be the case that the value is always castable
//to an integer
maxPoolSizeValue = Integer.parseInt(maxPoolSize);

This comment has been minimized.

Copy link
@MeroRai

MeroRai Jul 15, 2019

Contributor

This will cause NumberFormatException if the string does not have the appropriate numeric value.

default:
//We got a system variable as no split occured
if (systemProps.getProperty(variableReference[0]) != null && !systemProps.getProperty(variableReference[0]).isEmpty()) {
return systemProps.getProperty(variableReference[0]);

This comment has been minimized.

Copy link
@MeroRai

MeroRai Jul 15, 2019

Contributor

Can't you just simply use System.getProperty(variableToRetrieve) here.

@cubastanley

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2019

Jenkins Test Please

@cubastanley cubastanley requested a review from MeroRai Jul 17, 2019

@jbee
Copy link
Contributor

left a comment

Needs some cleanup.

switch(variableReference[0]) {
case "ENV":
//Check environment variables for requested value
if (System.getenv(variableReference[1]) != null && !System.getenv(variableReference[1]).isEmpty()) {

This comment has been minimized.

Copy link
@jbee

jbee Jul 17, 2019

Contributor

System.getenv(variableReference[1]) should be extracted to local var

case "MPCONFIG":
//Check microprofile config for requested value
Config config = ConfigProvider.getConfig();
if (config.getValue(variableReference[1], String.class) != null && !config.getValue(variableReference[1], String.class).isEmpty()) {

This comment has been minimized.

Copy link
@jbee

jbee Jul 17, 2019

Contributor

config.getValue(variableReference[1], String.class) should be extracted to local var

break;
default:
//We got a system variable as no split occured
if (System.getProperty(variableReference[0]) != null && !System.getProperty(variableReference[0]).isEmpty()) {

This comment has been minimized.

Copy link
@jbee

jbee Jul 17, 2019

Contributor

local var

if(steadyPoolSize == null) {

int maxPoolSizeValue = 0;
int steadyPoolSizeValue = 0;

This comment has been minimized.

Copy link
@jbee

jbee Jul 17, 2019

Contributor

It should be possible to add a helper function where the min/max/default and actual value go in and the successfully parsed or resolved value comes out that can be used for both of these. Essentially making a method that does the extended version of that the annotations did semantically.

@cubastanley cubastanley requested a review from jbee Jul 17, 2019

@cubastanley

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2019

Jenkins Test Please

@jbee
jbee approved these changes Jul 18, 2019

@cubastanley cubastanley merged commit 6730580 into payara:master Jul 18, 2019

59 checks passed

Payara Quick Build and Test Quick build and test passed!
Details
security/snyk - api/payara-api/pom.xml (payara-ci) No new issues
Details
security/snyk - api/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/admin/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/admingui/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ant-tasks/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/appclient/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/batch/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/common/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/concurrent/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/connectors/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/core/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/deployment/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/distributions/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ejb/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/extras/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/featuresets/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/flashlight/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/grizzly/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ha/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/installer/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/javaee-api/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/jdbc/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/jms/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/load-balancer/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/orb/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/osgi-platforms/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/packager/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/payara-appserver-modules/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/persistence/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/registration/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/resources/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/security/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/tests/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/transaction/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/web/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/webservices/pom.xml (payara-ci) No new issues
Details
security/snyk - copyright/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/admin/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/cluster/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/common/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/core/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/deployment/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/diagnostics/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/distributions/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/flashlight/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/grizzly/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/hk2/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/osgi-platforms/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/packager/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/payara-modules/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/resources-l10n/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/resources/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/security/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/test-utils/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/tests/pom.xml (payara-ci) No new issues
Details
security/snyk - pom.xml (payara-ci) No new issues
Details

@arjantijms arjantijms added this to the 5.193 milestone Jul 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.