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

implement getStatus for nrwrapper #1315

Merged

Conversation

tbradellis
Copy link
Contributor

@tbradellis tbradellis commented Jun 14, 2023

NRResponseWrapper class in our Servlet 5 and Servlet 6 instrumentation doesn't implement the getStatus method.
This came up in testing Jakarata 10 EE. Testing shows that the http.StatusCode method was not being added to agent attributes (an assertion on attribute map length revealed the issue).

The method in the wrapper always returns 0 causing the put to the attributes to be skipped.

@codecov-commenter
Copy link

codecov-commenter commented Jun 14, 2023

Codecov Report

Merging #1315 (61634cd) into main (aeb1c4e) will increase coverage by 0.01%.
The diff coverage is n/a.

❗ Current head 61634cd differs from pull request most recent head 1f9ac90. Consider uploading reports for the commit 1f9ac90 to get more accurate results

@@             Coverage Diff              @@
##               main    #1315      +/-   ##
============================================
+ Coverage     60.72%   60.73%   +0.01%     
- Complexity     8572     8573       +1     
============================================
  Files           856      856              
  Lines         40455    40455              
  Branches       6076     6076              
============================================
+ Hits          24565    24570       +5     
+ Misses        13410    13406       -4     
+ Partials       2480     2479       -1     

see 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@tbradellis tbradellis marked this pull request as draft June 14, 2023 03:52
@tbradellis tbradellis marked this pull request as ready for review June 14, 2023 04:03
@tbradellis tbradellis force-pushed the implement-getstatus-for-servlet-5-and-6-nrresponsewrapper branch from 4cb6a09 to 61634cd Compare June 14, 2023 04:07
add some code coverage for webrequestdispatcher

drop unrelated tests and do separate

optimize imports
@tbradellis tbradellis force-pushed the implement-getstatus-for-servlet-5-and-6-nrresponsewrapper branch from 61634cd to 1f9ac90 Compare June 14, 2023 04:09
@Override
public int getStatus() throws Exception {
return 0;
return response.getStatus();
Copy link
Contributor

Choose a reason for hiding this comment

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

Good find.

@tbradellis tbradellis merged commit 2be9238 into main Jun 14, 2023
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.

4 participants