jakartaee / platform

The Jakarta EE Platform project produces the Jakarta EE platform specification, which is an umbrella specification that aggregates all other Jakarta EE specifications.
https://jakartaee.github.io/platform/
Eclipse Public License 2.0
197 stars 66 forks source link

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

Open scottmarlow opened 3 months ago

scottmarlow commented 3 months ago

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.

bstansberry commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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

scottmarlow commented 2 months ago

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 commented 2 months ago

+1 on adding some tests

starksm64 commented 2 months ago

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);