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

Fix admin request crashing when timing responseSendTime is null #2275

Merged
merged 1 commit into from
Jul 19, 2023

Conversation

emilianoalvarez91
Copy link
Contributor

@emilianoalvarez91 emilianoalvarez91 commented Jul 17, 2023

The Timing class has 3 private properties addedDelay, processTime and responseSendTime of type Integer, however the accessors public functions return an int type. When any of these is null it causes a crash and this happened when calling http://127.0.0.1:{port}/__admin/requests.

2023-07-17 14:57:42.948 Unrecoverable error handling admin request
wiremock.com.fasterxml.jackson.databind.JsonMappingException: Cannot invoke "java.lang.Integer.intValue()" because "this.responseSendTime" is null (through reference chain: com.github.tomakehurst.wiremock.admin.model.GetServeEventsResult["requests"]->java.util.ArrayList[8]->com.github.tomakehurst.wiremock.stubbing.ServeEvent["timing"]->com.github.tomakehurst.wiremock.common.Timing["responseSendTime"])
Caused by: java.lang.NullPointerException: Cannot invoke "java.lang.Integer.intValue()" because "this.responseSendTime" is null
	at com.github.tomakehurst.wiremock.common.Timing.getResponseSendTime(Timing.java:58)

References

No reference

Submitter checklist

  • The PR request is well described and justified, including the body and the references
  • The PR title represents the desired changelog entry
  • The repository's code style is followed (see the contributing guide)
  • Test coverage that demonstrates that the change works as expected
  • For new features, there's necessary documentation in this pull request or in a subsequent PR to wiremock.org

@emilianoalvarez91 emilianoalvarez91 requested a review from a team as a code owner July 17, 2023 14:35
@tomakehurst
Copy link
Member

I think it would be better just to make these fields nullable. Returning 0 seems misleading to me.

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

Thanks for the patch! I agree with @tomakehurst . Returning 0 is legit for addedDelay but definitely creates confusion for other fields, null is definitely a better option in JSON responses

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

+1 from me. Formally it is an API breaking change, but I think it is reasonable in this case

@oleg-nenashev oleg-nenashev added the needs-tom Tom's Train Project :) label Jul 19, 2023
@oleg-nenashev oleg-nenashev merged commit 3ca1ee0 into wiremock:master Jul 19, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking change bug needs-tom Tom's Train Project :)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants