jakartaee / enterprise-beans

Jakarta Enterprise Beans
https://eclipse.org/ee4j/ejb
Other
20 stars 29 forks source link

[TCK] Read-only naming context and close() #138

Open jeanouii opened 3 years ago

jeanouii commented 3 years ago

Hi,

I was working on Apache TomEE TCK and investigating a failure. https://github.com/eclipse-ee4j/jakartaee-tck/blob/master/src/com/sun/ts/tests/ejb30/lite/naming/context/Client.java#L127

It calls this https://github.com/eclipse-ee4j/jakartaee-tck/blob/master/src/com/sun/ts/tests/ejb30/lite/naming/context/Test.java#L277

In essence, it was failing on TomEE because close was not allowed like bind for instance (close considered as changing the naming context)

The specification says https://jakarta.ee/specifications/enterprise-beans/4.0/jakarta-enterprise-beans-spec-core-4.0.html#container-provider-responsibility

I checked Tomcat https://github.com/apache/tomcat/blob/master/java/org/apache/naming/NamingContext.java#L758

It seems to also not allow close.

TCK seems to be considering close() as a read-only operation. The specification does not seem to make any difference for close().

The container must ensure that the enterprise bean instances have only read access to their environment variables. The container must throw the javax.naming.OperationNotSupportedException from all the methods of the javax.naming.Context interface that modify the environment naming context and its subcontexts.

Does this require an additional not in the spec to clarify what the TCK is actually testing? Or is the TCK behaviour questionable?

chengfang commented 3 years ago

I think the focus of this requirement is to disallow applications from modifying bean's environment through a Context, e.g., by calling its bind, rebind or other similar methods. Application should have the ability to bring a Context into existence (through instantiation or lookup), and dispose of such a Context when done (by calling Context.close() method). The disposition of a Context does not modify a bean's environment.

From a practical point of view, applications need such a way to indicate to the container that the Context is no longer needed and the container can free associated resources.

https://docs.oracle.com/javase/8/docs/api/javax/naming/Context.html#close--

So I would say it's a valid test.

Cheng

On Fri, Dec 18, 2020 at 4:55 AM Jean-Louis Monteiro < notifications@github.com> wrote:

Hi,

I was working on Apache TomEE TCK and investigating a failure.

https://github.com/eclipse-ee4j/jakartaee-tck/blob/master/src/com/sun/ts/tests/ejb30/lite/naming/context/Client.java#L127

It calls this

https://github.com/eclipse-ee4j/jakartaee-tck/blob/master/src/com/sun/ts/tests/ejb30/lite/naming/context/Test.java#L277

In essence, it was failing on TomEE because close was not allowed like bind for instance (close considered as changing the naming context)

The specification says

https://jakarta.ee/specifications/enterprise-beans/4.0/jakarta-enterprise-beans-spec-core-4.0.html#container-provider-responsibility

I checked Tomcat

https://github.com/apache/tomcat/blob/master/java/org/apache/naming/NamingContext.java#L758

It seems to also not allow close.

TCK seems to be considering close() as a read-only operation. The specification does not seem to make any difference for close().

The container must ensure that the enterprise bean instances have only read access to their environment variables. The container must throw the javax.naming.OperationNotSupportedException from all the methods of the javax.naming.Context interface that modify the environment naming context and its subcontexts.

Does this require an additional not in the spec to clarify what the TCK is actually testing? Or is the TCK behaviour questionable?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/eclipse-ee4j/ejb-api/issues/138, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAP3UEYU33BJ3T7GEI4FYWTSVMRIJANCNFSM4VA6A76Q .

jeanouii commented 3 years ago

Thanks for the quick response.

The disposition of a Context does not modify a bean's environment.

This is effectively what the test is doing. And the test seems to be inline with your reading as well. I was more asking because I realised at least 2 implementations were having a different reading and were considering the close() operation to be modifying the bean's environment.

I fixed already Apache TomEE to relax a bit the constraints.

What I would probably suggest is add a small mention to the close() operation in the spec and API javadoc, so it's cristal clear, close() does not modify the bean's environment.

chengfang commented 3 years ago

+1 for clarifyingthe close() in the spec. API javadoc belongs to Java SE so we probably don't want to modify it.

This applies not only to ejb, but also to other components, so EE platform spec may be a better place to have this clarification.

jeanouii commented 3 years ago

Agreed, thanks