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

Add additional guards for cases where AuditLogger params may be null #370

Merged
merged 1 commit into from
Jun 21, 2017

Conversation

rnorth
Copy link
Member

@rnorth rnorth commented Jun 14, 2017

Also add @Nullable/@NotNull annotations to enable detection of bugs in future

@rnorth rnorth requested review from bsideup and kiview June 14, 2017 21:08
@rnorth rnorth modified the milestone: 1.3.1 Jun 14, 2017
Also add @Nullable/@NotNull annotations to enable detection of bugs in future
@rnorth rnorth force-pushed the audit-logger-defensive-checks branch from c49acbd to 9c8b361 Compare June 14, 2017 21:21
@NotNull DockerCmd<?> cmd,
@Nullable Exception e) {

if (! log.isTraceEnabled()) {
Copy link
Member

Choose a reason for hiding this comment

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

What if we make this audit thing completely optional?

i.e. return AuditClient here (

) only if some property is set. This way we are safe from any issue related to it, including MDC NPE handling, reflection, etc :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Fine, OK 👍
Let's merge #362 first to avoid spending too much time resolving conflicts on the TestcontainersConfiguration class

Copy link
Member

Choose a reason for hiding this comment

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

@rnorth I'm thinking what if we merge it now, release (because users are affected) and make it optional in a next release?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, let's do it.

@rnorth rnorth merged commit 6411ce5 into master Jun 21, 2017
@rnorth rnorth deleted the audit-logger-defensive-checks branch June 21, 2017 05:43
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.

2 participants