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

Facade for ServletRequest which may not be on the classpath and related cleanups #162

Merged
merged 2 commits into from
Mar 10, 2022

Conversation

dmatej
Copy link
Contributor

@dmatej dmatej commented Mar 10, 2022

Based on discussion from #161 this should be merged after merging PR #161

  • previous impl used reflection and java.lang.Object, but in all javadocs mentioned that the Object must be ServletContext
  • this is more strict, uses ServletContext directly, but as it still may not be available, it is represented by the facade, or null is provided, which is same as before.
  • removed reflection including related error messages
  • impl moved out of SecurityUtil where it doesn't belong

@dmatej
Copy link
Contributor Author

dmatej commented Mar 10, 2022

Glassfish build including tests directly in build:

[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  10:58 min
[INFO] Finished at: 2022-03-10T16:21:00+01:00
[INFO] ------------------------------------------------------------------------

Glassfish's old security_all tests:

[exec] ************************
[exec] PASSED=   724
[exec] ------------  =========
[exec] FAILED=   0
[exec] ------------  =========
[exec] DID NOT RUN=   75
[exec] ------------  =========
[exec] Total Expected=799
[exec] ************************
[exec]

BUILD SUCCESSFUL
Total time: 7 minutes 10 seconds

Glassfish's old webservice_all tests:

[java] **********************
[java] * PASSED          81 *
[java] * FAILED           0 *
[java] * DID NOT RUN      0 *
[java] * TOTAL           81 *
[java] **********************
[java] 

BUILD SUCCESSFUL
Total time: 9 minutes 22 seconds

@dmatej
Copy link
Contributor Author

dmatej commented Mar 10, 2022

That Tomcat failure is caused by the fact that Tomcat released M11 and M10 was removed ...
https://dlcdn.apache.org/tomcat/tomcat-10/v10.1.0-M11/bin/

@lukasj
Copy link
Member

lukasj commented Mar 10, 2022

can you update it in https://github.com/eclipse-ee4j/metro-wsit/blob/master/.github/workflows/maven.yml#L86-L89, please?

@dmatej
Copy link
Contributor Author

dmatej commented Mar 10, 2022

Locally build passed with M11.
The dependency update is here: #163

…ed cleanups

- previous impl used reflection and java.lang.Object, but in all javadocs
  mentioned that the Object must be ServletContext
- this is more strict, uses ServletContext directly, but as it still may
  not be available, it is represented by the facade, or null is provided,
  which is same as before.
- removed reflection including related error messages
- impl moved out of SecurityUtil where it doesn't belong
@@ -36,6 +37,7 @@
import com.sun.xml.wss.XWSSecurityException;
import com.sun.xml.wss.impl.MessageConstants;
import com.sun.xml.wss.logging.LogDomainConstants;
import com.sun.xml.wss.logging.impl.crypto.LogStringsMessages;
Copy link
Member

Choose a reason for hiding this comment

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

the rule is simple:

if a class is in package com.sun.xml.wss.impl.misc, keys come from the resource in com.sun.xml.wss.logging.impl.misc; if a class is in package com.sun.xml.wss.impl.crypto, keys come from the resource in com.sun.xml.wss.logging.impl.crypto - unless some specific message is shared in multiple places

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would start thinking about moving this class somewhere else - crypto? security? own package? Depends on class and package dependencies, I did not investigate too much, but "misc" is not the right package for this specialized utility class. Maybe it would make even sense to first remove methods if they are used just from one concrete place. That would help to get rid of part of dependencies and have a clear view on what remained.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it would make even sense to first remove methods if they are used just from one concrete place.

that would be good start, I don't mind if the same code is copy&pasted on 2 places (3+ is just too much...) should it help

Copy link
Member

@lukasj lukasj left a comment

Choose a reason for hiding this comment

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

LGTM

note that the nightly build is going to fail

@lukasj lukasj merged commit 6f4bd61 into eclipse-ee4j:master Mar 10, 2022
@lukasj
Copy link
Member

lukasj commented Mar 10, 2022

note that the second commit should be backported to EE8 branch

* https://www.gnu.org/software/classpath/license.html.
*
* SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
*/
Copy link
Member

Choose a reason for hiding this comment

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

This license is WRONG - must be EDL as is the rest of the code base. I'll change it.

Copy link
Member

Choose a reason for hiding this comment

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

btw: nightly build job is saying that in https://ci.eclipse.org/metro/job/wsit-master-build/124/console (it's linked from the project home page) and notification was sent to metro-dev list

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aha, I had a discussion with Arjan yesterday, he noticed somewhere that the copyright plugin takes just the first line to parse the year, so the latest should be first. Until now I always added just the second line.
Sorry for the license, I forgot it is different from Glassfish.

Copy link
Member

Choose a reason for hiding this comment

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

it can check multiple things - for the year you are correct, it takes only the first line (unless copyright.ignoreyear is true), but it also checks the license as such either against arbitrary template (copyright.template or copyright.alternateTemplate (or sth similar) properties) or against some predefined ones packaged within the copyright plugin

@dmatej dmatej deleted the soft-code-cleanup branch March 11, 2022 11:28
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.

None yet

2 participants