[BUG-3581] [Code Style] HttpStatus & Headers should not be imported from `org.apache.hc.core5.http` #3663

prabhask5 · 2023-11-07T02:33:59Z

Description

Accounts for refactor of Header and HttpStatus classes from hc.cor5.http to just http by adding checkstyle test, and manually refactoring all current occurrences of the illegal import. Additionally updated spotlessApply to make the refactor automatic on future cases of illegal import.

Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation)

Bug fix/refactor

Why these changes are required?

Refer to #3581

What is the old behavior before changes and new behavior after changes?

Ideally nothing.

Issues Resolved

[BUG] [Code Style] HttpStatus & Headers should not be imported from org.apache.hc.core5.http #3581

Is this a backport? If so, please add backport PR # and/or commits #
No.

Testing

Tests have been modified and should work before merging.

Check List

New functionality includes testing
New functionality has been documented
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Prabhas Kurapati <[email protected]>

prabhask5 · 2023-11-07T03:35:26Z

@cwperks @scrawfor99 @DarshitChanpura @peternied Through updating the illegal import org.apache.hc.core5.http.Header, to org.apache.http.Header I'm getting a lot of type compilation errors that try unsuccessfully to cast org.apache.hc.core5.http.Header to org.apache.http.Header. Here is an example of one such error: /Users/prabhask/Documents/Internships/opensearch-security/src/test/java/org/opensearch/security/test/helper/rest/RestHelper.java:238: error: incompatible types: org.apache.http.Header cannot be converted to org.apache.hc.core5.http.Header

To solve this problem I can either manually add a cast to org.apache.hc.core5.http.Header whenever this error appears or change the imports of all the dependent classes to also exclude core5 or client5, both of which seem like bad solutions. Is there another way to solve this problem?

reta · 2023-11-07T12:38:49Z

@cwperks @scrawfor99 @DarshitChanpura @peternied Through updating the illegal import org.apache.hc.core5.http.Header,

@prabhask5 why org.apache.hc.core5.http.Header is illegal? The main branch uses Apache HttpClient 5 and should import these packages, the 2.x uses Apache HttpClient 4.x and should import org.apache.http.Header, there should be no attempts to do any kind of type casting.

prabhask5 · 2023-11-07T20:37:53Z

@cwperks @scrawfor99 @DarshitChanpura @peternied Through updating the illegal import org.apache.hc.core5.http.Header,

@prabhask5 why org.apache.hc.core5.http.Header is illegal? The main branch uses Apache HttpClient 5 and should import these packages, the 2.x uses Apache HttpClient 4.x and should import org.apache.http.Header, there should be no attempts to do any kind of type casting.

I'm referencing #3581 , if its not illegal I might be misunderstanding what the issue is saying.

reta · 2023-11-07T20:45:02Z

I'm referencing #3581 , if its not illegal I might be misunderstanding what the issue is saying.

Me neither, the security plugin uses RestClient which directly exports some of the Apache HttpClient 5 or 4.x APIs (depending on the branch).

stephen-crawford · 2023-11-07T21:36:57Z

@prabhask5 thanks for being proactive on this.

Similar to @reta:

I am not sure what the original issue was referencing. I don't think that it will be possible to standardize the versions without significant refactoring. You will need to swap to the v4 equivalents for everything in main.

Eventually we will move to 3.0 and likely swap completely to v5 when we do.

I don't think it makes sense to spend too much time converting the versions back and forth.

prabhask5 · 2023-11-07T21:56:39Z

@prabhask5 thanks for being proactive on this.

Similar to @reta:

I am not sure what the original issue was referencing. I don't think that it will be possible to standardize the versions without significant refactoring. You will need to swap to the v4 equivalents for everything in main.

Eventually we will move to 3.0 and likely swap completely to v5 when we do.

I don't think it makes sense to spend too much time converting the versions back and forth.

So is this issue irrelevant then? @peternied Do you have some insight on this?

peternied · 2023-11-07T23:57:09Z

Illegal has weighty connotations - I'd prefer to think of direct imports of ...hc.core5... as not advisable. The rational is that it makes our great auto-backporting tool not work for nearly any case where we are authoring test cases (which I hope is often!). There is more context in the issue [1].

@reta What do you think about this rational? After we release 3.0.0, we will still be supporting 2.x builds for a least a year, anything that helps make the backport process result in less merge conflicts is a solid win to me.

[1] [BUG] [Code Style] HttpStatus & Headers should not be imported from org.apache.hc.core5.http #3581

peternied · 2023-11-07T23:57:58Z

@prabhask5 Looks like there are many merge conflicts that you'll need to resolve - I'd suggest a git merge origin/main YMMV

prabhask5 · 2023-11-08T08:42:44Z

@prabhask5 Looks like there are many merge conflicts that you'll need to resolve - I'd suggest a git merge origin/main YMMV

@peternied Sounds good. How do you recommend fixing the errors I'm getting? I have them copied below for convenience.

Through updating the illegal import org.apache.hc.core5.http.Header, to org.apache.http.Header I'm getting a lot of type compilation errors that try unsuccessfully to cast org.apache.hc.core5.http.Header to org.apache.http.Header. Here is an example of one such error: /Users/prabhask/Documents/Internships/opensearch-security/src/test/java/org/opensearch/security/test/helper/rest/RestHelper.java:238: error: incompatible types: org.apache.http.Header cannot be converted to org.apache.hc.core5.http.Header

To solve this problem I can either manually add a cast to org.apache.hc.core5.http.Header whenever this error appears or change the imports of all the dependent classes to also exclude core5 or client5, both of which seem like bad solutions. Is there another way to solve this problem?

…-hc-core5-import

peternied · 2023-11-08T20:59:28Z

org.apache.hc.core5.http.Header and org.apache.http.Header are different implementations, they won't translate between one another. There are a couple of options to handle this:

Don't attempt to convert these objects. While it isn't idea,l if the majority of the time we are using status codes and header constants ARE converted, that solves the majority problem.
Create a shared interface / use another object at object declartions. Headers aren't very complex, it would be relatively easy to create or use another object that represented this key value pairing, then convert it in the RestHelper where it needs to be type specific.
An even better solution some invents.

prabhask5 · 2023-11-09T09:07:25Z

org.apache.hc.core5.http.Header and org.apache.http.Header are different implementations, they won't translate between one another. There are a couple of options to handle this:

Don't attempt to convert these objects. While it isn't idea,l if the majority of the time we are using status codes and header constants ARE converted, that solves the majority problem.

Create a shared interface / use another object at object declartions. Headers aren't very complex, it would be relatively easy to create or use another object that represented this key value pairing, then convert it in the RestHelper where it needs to be type specific.

An even better solution some invents.

@peternied I'm pretty confused on these solutions. The majority of the errors I'm getting are either functions where the new Header is not an applicable type for the parameter, but I can't edit these functions since they are in the Apache codebase. How should I fix these errors? The only solution I'm seeing is to manually cast to the new type of Header, which again I don't think is advisable. A more specific solution would help a lot, ty!

peternied · 2023-11-09T16:58:06Z

@prabhask5 I might recommend going with the first option, remove changes for Headers for this PR. After the status codes have been aligned take a single file and see what it takes to get it working with a draft PR.

This change is already massive and providing feedback is non-trivial if you are running into build/runtime issues.

Signed-off-by: Prabhas Kurapati <[email protected]>

prabhask5 · 2023-11-14T07:14:22Z

@prabhask5 I might recommend going with the first option, remove changes for Headers for this PR. After the status codes have been aligned take a single file and see what it takes to get it working with a draft PR.

This change is already massive and providing feedback is non-trivial if you are running into build/runtime issues.

@peternied Done! Feel free to unblock tests.

DarshitChanpura

I see mixed usages of 4 & 5. Also some classes still import Header from 5 vs 4, is that expected?
@reta thoughts?

DarshitChanpura · 2023-11-14T20:04:56Z

src/integrationTest/java/org/opensearch/security/http/JwtAuthenticationTests.java

@@ -16,6 +16,8 @@
 import java.util.Map;

 import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope;
+import io.jsonwebtoken.SignatureAlgorithm;
+import io.jsonwebtoken.security.Keys;
 import org.apache.hc.core5.http.Header;
 import org.apache.hc.core5.http.message.BasicHeader;


this seems to be unrelated. The original import for hc5 still remains as is

DarshitChanpura · 2023-11-14T20:05:30Z

src/integrationTest/java/org/opensearch/security/http/OnBehalfOfJwtAuthenticationTest.java

+import io.jsonwebtoken.Claims;
+import io.jsonwebtoken.Jwts;
+import io.jsonwebtoken.security.Keys;
+
 import org.apache.hc.core5.http.Header;


why only 1 of these 2 changed?

DarshitChanpura · 2023-11-14T20:07:13Z

src/integrationTest/java/org/opensearch/security/rest/CompressionTests.java

@@ -20,7 +20,7 @@
 import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope;
 import org.apache.hc.client5.http.classic.methods.HttpPost;
 import org.apache.hc.core5.http.ContentType;
-import org.apache.hc.core5.http.HttpStatus;
+import org.apache.http.HttpStatus;


should this be a mixed usage between 4 & 5?

DarshitChanpura · 2023-11-14T20:07:46Z

src/integrationTest/java/org/opensearch/security/rest/WhoAmITests.java

@@ -22,7 +22,8 @@
 import java.util.stream.Collectors;

 import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope;
-import org.apache.hc.core5.http.HttpStatus;
+import joptsimple.internal.Strings;


what is this import? is it being used?

DarshitChanpura · 2023-11-14T20:11:27Z

src/test/java/org/opensearch/security/system_indices/AbstractSystemIndicesTests.java

@@ -15,6 +15,7 @@
 import java.util.List;

 import org.apache.hc.core5.http.Header;
+


nit: seems like this import was not replaced

DarshitChanpura · 2023-11-14T20:12:00Z

src/test/java/org/opensearch/security/system_indices/SystemIndexDisabledTests.java

@@ -11,10 +11,8 @@

 package org.opensearch.security.system_indices;

-import java.io.IOException;
-
 import org.apache.hc.core5.http.Header;


should this be org.apache.http.HttpStatus

Signed-off-by: Prabhas Kurapati <[email protected]>

prabhask5 · 2023-11-19T10:24:33Z

Closed PR due to git issues new PR here: #3741

prabhask5 added 6 commits November 6, 2023 17:47

modified checkstyle xml file

7ebb723

Signed-off-by: Prabhas Kurapati <[email protected]>

refactored illegal imports

a43130c

Signed-off-by: Prabhas Kurapati <[email protected]>

fixed checkstyle xml

8fe2ebf

Signed-off-by: Prabhas Kurapati <[email protected]>

revert illegal import refactor commit

2cc6bdb

Signed-off-by: Prabhas Kurapati <[email protected]>

refactored illegal HttpStatus import

58e74ec

Signed-off-by: Prabhas Kurapati <[email protected]>

refactor illegal Header import

88e38c2

Signed-off-by: Prabhas Kurapati <[email protected]>

prabhask5 changed the title ~~Pk refactor illegal hc core5 import~~ [BUG-3581] [Code Style] HttpStatus & Headers should not be imported from org.apache.hc.core5.http Nov 7, 2023

update spotless apply

9eed6ff

Signed-off-by: Prabhas Kurapati <[email protected]>

prabhask5 marked this pull request as ready for review November 7, 2023 03:35

prabhask5 requested review from cliu123, cwperks, DarshitChanpura, davidlago, peternied, RyanL1997, stephen-crawford, reta and willyborankin as code owners November 7, 2023 03:35

Merge remote-tracking branch 'upstream/main' into pk-refactor-illegal…

2809572

…-hc-core5-import

prabhask5 added 3 commits November 13, 2023 23:06

revert header refactor

b3ec2a5

Signed-off-by: Prabhas Kurapati <[email protected]>

update checkstyle + fix merge errors

95780de

Signed-off-by: Prabhas Kurapati <[email protected]>

fix checkstyle errors

3c43778

Signed-off-by: Prabhas Kurapati <[email protected]>

DarshitChanpura reviewed Nov 14, 2023

View reviewed changes

revert all changes

a5ef4bf

Signed-off-by: Prabhas Kurapati <[email protected]>

prabhask5 closed this Nov 19, 2023

prabhask5 deleted the pk-refactor-illegal-hc-core5-import branch November 19, 2023 09:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUG-3581] [Code Style] HttpStatus & Headers should not be imported from `org.apache.hc.core5.http` #3663

[BUG-3581] [Code Style] HttpStatus & Headers should not be imported from `org.apache.hc.core5.http` #3663

prabhask5 commented Nov 7, 2023 •

edited by peternied

Loading

prabhask5 commented Nov 7, 2023

reta commented Nov 7, 2023

prabhask5 commented Nov 7, 2023

reta commented Nov 7, 2023

stephen-crawford commented Nov 7, 2023

prabhask5 commented Nov 7, 2023

peternied commented Nov 7, 2023

peternied commented Nov 7, 2023

prabhask5 commented Nov 8, 2023

peternied commented Nov 8, 2023

prabhask5 commented Nov 9, 2023

peternied commented Nov 9, 2023

prabhask5 commented Nov 14, 2023 •

edited

Loading

DarshitChanpura left a comment

DarshitChanpura Nov 14, 2023

DarshitChanpura Nov 14, 2023

DarshitChanpura Nov 14, 2023

DarshitChanpura Nov 14, 2023

DarshitChanpura Nov 14, 2023

DarshitChanpura Nov 14, 2023

prabhask5 commented Nov 19, 2023

		@@ -15,6 +15,7 @@
		import java.util.List;

		import org.apache.hc.core5.http.Header;

[BUG-3581] [Code Style] HttpStatus & Headers should not be imported from org.apache.hc.core5.http #3663

[BUG-3581] [Code Style] HttpStatus & Headers should not be imported from org.apache.hc.core5.http #3663

Conversation

prabhask5 commented Nov 7, 2023 • edited by peternied Loading

Description

Issues Resolved

Testing

Check List

prabhask5 commented Nov 7, 2023

reta commented Nov 7, 2023

prabhask5 commented Nov 7, 2023

reta commented Nov 7, 2023

stephen-crawford commented Nov 7, 2023

prabhask5 commented Nov 7, 2023

peternied commented Nov 7, 2023

peternied commented Nov 7, 2023

prabhask5 commented Nov 8, 2023

peternied commented Nov 8, 2023

prabhask5 commented Nov 9, 2023

peternied commented Nov 9, 2023

prabhask5 commented Nov 14, 2023 • edited Loading

DarshitChanpura left a comment

Choose a reason for hiding this comment

DarshitChanpura Nov 14, 2023

Choose a reason for hiding this comment

DarshitChanpura Nov 14, 2023

Choose a reason for hiding this comment

DarshitChanpura Nov 14, 2023

Choose a reason for hiding this comment

DarshitChanpura Nov 14, 2023

Choose a reason for hiding this comment

DarshitChanpura Nov 14, 2023

Choose a reason for hiding this comment

DarshitChanpura Nov 14, 2023

Choose a reason for hiding this comment

prabhask5 commented Nov 19, 2023

[BUG-3581] [Code Style] HttpStatus & Headers should not be imported from `org.apache.hc.core5.http` #3663

[BUG-3581] [Code Style] HttpStatus & Headers should not be imported from `org.apache.hc.core5.http` #3663

prabhask5 commented Nov 7, 2023 •

edited by peternied

Loading

prabhask5 commented Nov 14, 2023 •

edited

Loading