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

Should @DataSourceDefinition be validated by the Jakarta EE 11 Platform TCK? #907

Open
scottmarlow opened this issue Jun 19, 2024 · 7 comments
Labels
EE11 Jakarta EE 11 Release

Comments

@scottmarlow
Copy link
Contributor

As per https://jakarta.ee/specifications/platform/10/jakarta-platform-spec-10.0#a1688 regarding DataSourceDefinition use in production:

(Of course, we do not recommend including passwords to production systems in the code, but it’s often useful while testing. Passwords, or other parts of the DataSource definition, can be overridden by a deployment descriptor when the application is deployed.)

Should the @DataSourceDefinition feature be validated by the Jakarta EE 11 Platform TCK?

If no, we should remove @DataSourceDefinition testing from the EE 11 Platform TCK.

If yes, that may be hard to test since the test source that contains the @DataSourceDefinition must be recompiled after the test source is updated to include database server connectivity information. In EE 10, we do recompile the test source after it is updated to include database passwords and other database server connectivity information.

@scottmarlow scottmarlow added the EE11 Jakarta EE 11 Release label Jun 19, 2024
@bstansberry
Copy link
Contributor

If yes, that may be hard to test since the test source that contains the @DataSourceDefinition must be recompiled after the test source is updated to include database server connectivity information. In EE 10, we do recompile the test source after it is updated to include database passwords and other database server connectivity information.

I'm confused by this. On the one hand, it will be hard to test, but on the other hand it seems it's already tested for EE 10. So I'm not sure how to weigh this 'hard to test' factor.

On the general point I don't see the 'we do not recommend' wording as a reason to drop testing of DataSourceDefinition altogether. It's an EE feature that's used. From a purely WildFly/EAP standpoint, we support using secure expressions as annotation attribute values, so the username/password need not be in the code, so it's possible to securely use this feature.

@scottmarlow
Copy link
Contributor Author

scottmarlow commented Jul 1, 2024

If yes, that may be hard to test since the test source that contains the @DataSourceDefinition must be recompiled after the test source is updated to include database server connectivity information. In EE 10, we do recompile the test source after it is updated to include database passwords and other database server connectivity information.

I'm confused by this. On the one hand, it will be hard to test, but on the other hand it seems it's already tested for EE 10. So I'm not sure how to weigh this 'hard to test' factor.

Copying an example DataSourceDefinition from https://github.com/jakartaee/platform-tck/blob/10.0.x/src/com/sun/ts/tests/appclient/deploy/metadatacomplete/testapp/files/TestAppClient.java.src#L49:

@DataSourceDefinition(name="java:global/MyApp/MyDataSource",
    className="@dbclassname@",
    url="@dburl@",
    user="@dbuser@",
    password="@dbpassword@"
 )

The above fragment is from an Jakarta EE 10 Platform TCK source file named TestAppClient.java.src which currently depends on parameters like @dbclassname@ to be replaced with a property that is configured by the person running the TCK test and then some scripting to recompile the test. There are some ideas for how to do equivalent testing with the EE 11 Platform TCK but the Platform description of DataSourceDefinition (https://jakarta.ee/specifications/platform/10/jakarta-platform-spec-10.0#a1688) mentions:
(Of course, we do not recommend including passwords to production systems in the code, but it’s often useful while testing. Passwords, or other parts of the DataSource definition, can be overridden by a deployment descriptor when the application is deployed.)

So, it would be easy for the EE 11 Platform TCK to provide a test source project that users can build + run after updating the @DataSourceDefinition with database connectivity information that will work for them. There are likely other (easy) ways to test @DataSourceDefinition.

On the general point I don't see the 'we do not recommend' wording as a reason to drop testing of DataSourceDefinition altogether. It's an EE feature that's used. From a purely WildFly/EAP standpoint, we support using secure expressions as annotation attribute values, so the username/password need not be in the code, so it's possible to securely use this feature.

Agreed as over the years, there has been a lot of passion about keeping @DataSourceDefinition but I'd still like a portable way to easily test the feature.

@hantsy
Copy link

hantsy commented Jul 2, 2024

It is better to support standard EL in these attributes across multiple application servers. Currently, Payara and WildFly use their format for attribute EL configuration.

@hantsy
Copy link

hantsy commented Jul 2, 2024

I filed an issue on common-annotations before, check here: jakartaee/common-annotations-api#95, but no plan for this.

@scottmarlow
Copy link
Contributor Author

Thanks @bstansberry @hantsy for the feedback!

From the discussion here I see interest in using expressions in @DataSourceDefinition(s) in Jakarta EE 11 application source code. If/when that becomes a feature for Jakarta EE , that would need to be tested via a Platform TCK test that all EE implementations must pass.

Should the @DataSourceDefinition feature be validated by the Jakarta EE 11 Platform TCK?

No one responded that the answer is no, so I think by default the answer is yes.

If no, we should remove @DataSourceDefinition testing from the EE 11 Platform TCK.

Lets keep this issue open a bit longer to see if we get additional feedback.

If yes, that may be hard to test since the test source that contains the @DataSourceDefinition must be recompiled after the test source is updated to include database server connectivity information. In EE 10, we do recompile the test source after it is updated to include database passwords and other database server connectivity information.

I think the simplest @DataSourceDefinition TCK test would be to specify a JDBC driver class that mocks a JDBC driver, perhaps something like:

@DataSourceDefinition(name = "java:global/MyApp/MyDataSource", className = "org.mock.jdbcdriver", url = "mock@localhost", user = "testuser", password = "testpassword")

@Emily-Jiang
Copy link
Contributor

+1 on adding some tests

@starksm64
Copy link
Contributor

starksm64 commented Jul 11, 2024

There are tests. Searching the current platform-tck tckrefactor branch shows the following usage:

Targets
    Occurrences of 'DataSourceDefinition' in Directory /home/starksm/Dev/Jakarta/sms-platform-tck with mask '*.java'
Found occurrences in Directory /home/starksm/Dev/Jakarta/sms-platform-tck with mask '*.java'  (19 usages found)
    Production  (19 usages found)
        Unclassified  (15 usages found)
            appclient  (3 usages found)
                com.sun.ts.tests.appclient.deploy.metadatacomplete.testapp  (3 usages found)
                    TestAppClient.java  (3 usages found)
                        testDataSourceDefinitionAnnotation()  (1 usage found)
                            273 public void testDataSourceDefinitionAnnotation() throws Exception {
                        31 import jakarta.annotation.sql.DataSourceDefinition;
                        45 @DataSourceDefinition(name = "java:global/MyApp/MyDataSource", className = "oracle.jdbc.pool.OracleDataSource", url = "jdbc:oracle:thin:@localhost:1521:orcl", user = "TESTU", password = "TESTU")
            ejb30  (12 usages found)
                com.sun.ts.tests.ejb30.lite.packaging.war.datasource.stateful  (4 usages found)
                    DataSourceBean.java  (2 usages found)
                        33 import jakarta.annotation.sql.DataSourceDefinition;
                        40 @DataSourceDefinition(name = "java:app/env/appds2", description = "override with <data-source> in ejb-jar.xml", className
                    Interceptor1.java  (2 usages found)
                        32 import jakarta.annotation.sql.DataSourceDefinition;
                        36 @DataSourceDefinition(name = "java:module/env/moduleds", description = "override with <data-source> in ejb-jar.xml", className
                com.sun.ts.tests.ejb30.misc.datasource.twojars  (8 usages found)
                    Client.java  (4 usages found)
                        32 import jakarta.annotation.sql.DataSourceDefinition;
                        33 import jakarta.annotation.sql.DataSourceDefinitions;
                        36 @DataSourceDefinitions({
                        37 @DataSourceDefinition(name = "java:global/datasource/twojars/appclient/globalds", description = "override it with <data-source
                    DataSource2Bean.java  (4 usages found)
                        30 import jakarta.annotation.sql.DataSourceDefinition;
                        31 import jakarta.annotation.sql.DataSourceDefinitions;
                        37 @DataSourceDefinitions({
                        38 @DataSourceDefinition(name = "java:global/datasource/twojars/2/globalds", description = "override it with <data-source> in
        Usage in comments  (2 usages found)
            appclient  (2 usages found)
                com.sun.ts.tests.appclient.deploy.metadatacomplete.testapp  (2 usages found)
                    TestAppClient.java  (2 usages found)
                        testDataSourceDefinitionAnnotation()  (2 usages found)
                            259 * @testName: testDataSourceDefinitionAnnotation
                            268 *                 true,DataSourceDefinition annotation should be ignored - as
        Usage in string constants  (2 usages found)
            appclient  (2 usages found)
                com.sun.ts.tests.appclient.deploy.metadatacomplete.testapp  (2 usages found)
                    TestAppClient.java  (2 usages found)
                        testDataSourceDefinitionAnnotation()  (2 usages found)
                            277 throw new Exception("DataSourceDefinition test failed!");
                            280 throw new Exception("DataSourceDefinition test failed: " + e, e);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
EE11 Jakarta EE 11 Release
Projects
None yet
Development

No branches or pull requests

5 participants