Skip to content

Conversation

@vpavic
Copy link
Contributor

@vpavic vpavic commented May 3, 2016

I've recently had some requirements on reporting/handling audit events (which included retrieving the latest events on the system, retrieving events of a specific type for a given user), so it seemed fit to consider the expansion of AuditEventRepository.

Investigating the default implementation (InMemoryAuditEventRepository) the following things didn't sit well with me:

  • InMemoryAuditEventRepository#find(String, Date) allows null principal to be used as wildcard for finding events for all users (which is even more confusing considering the fact that AuditEvent constructor allows null principal, but that's another story) rather than using separate find operation
  • InMemoryAuditEventRepository#add operation allows passing null events, which effectively breaks the aforementioned find operation since it relies on null values to indicate the end of its internal circular buffer

This PR adds the following changes to improve the situation:

  • add dedicated operation to AuditEventRepository for retrieving events without filtering by principal
  • add dedicated operation to AuditEventRepository for retrieving events filtered by principal and type of event
  • validate InMemoryAuditEventRepository#add input to prevent nulls
  • validate construction of AuditEvent to prevent null principals
  • improve AuditEvent test coverage

Further improvements to consider:

  • name AuditEventRepository finders to describe use case (this could include deprecating current find)
  • remove InMemoryAuditEventRepository#setCapacity and enforce setting capacity only via constructor (note that using setCapacity drops the current buffer if used in runtime)
  • use object instead of method synchronization in InMemoryAuditEventRepository
  • introduce a global value for an unknown principal (see AuthorizationAuditListener:50)

Thoughts?

  • I have signed the CLA

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 3, 2016
@vpavic vpavic force-pushed the audit-repository branch 2 times, most recently from f870d95 to c592fb9 Compare May 4, 2016 17:15
@vpavic vpavic force-pushed the audit-repository branch from c592fb9 to d47683a Compare May 5, 2016 05:28
@philwebb philwebb added the for: team-attention An issue we'd like other members of the team to review label May 13, 2016
@philwebb philwebb removed for: team-attention An issue we'd like other members of the team to review status: waiting-for-triage An issue we've not yet triaged labels Jun 22, 2016
@philwebb philwebb added this to the 1.4.0.RC1 milestone Jun 22, 2016
@philwebb philwebb added the type: enhancement A general enhancement label Jun 22, 2016
@snicoll snicoll self-assigned this Jun 30, 2016
snicoll pushed a commit that referenced this pull request Jun 30, 2016
@snicoll snicoll closed this in 3ba2b24 Jun 30, 2016
snicoll added a commit that referenced this pull request Jun 30, 2016
* pr/5854:
  Polish "Improve AuditEventRepository"
  Improve AuditEventRepository
philwebb added a commit that referenced this pull request Jul 1, 2016
Update AuditEventRepository to restore support for `null` arguments and
explicitly Javadoc their meaning.

See gh-5854
@philwebb
Copy link
Member

philwebb commented Jul 1, 2016

Looking at these changes a little more, I'm worried that we're going to break back compatibility if we drop support for calling find with null parameters. I've updated the Javadoc for AuditEventRepository to say that null values are allowed (and what they mean) and updated InMemoryAuditEventRepository to support them.

@wilkinsona
Copy link
Member

3ba2b24 lost all of the synchronization improvements. It would be nice to keep those.

@wilkinsona wilkinsona reopened this Jul 1, 2016
@wilkinsona wilkinsona assigned wilkinsona and unassigned snicoll Jul 1, 2016
@snicoll
Copy link
Member

snicoll commented Jul 1, 2016

Sorry about that Andy. I just realized the mistake I've made 😶

@wilkinsona
Copy link
Member

I'm going to take care of the synchronisation changes in #6261

@wilkinsona wilkinsona closed this Jul 1, 2016
@vpavic vpavic deleted the audit-repository branch July 1, 2016 12:37
@vpavic
Copy link
Contributor Author

vpavic commented Jul 1, 2016

Thanks for merging this! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: enhancement A general enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants