From 5f19323fbd25edd530a387d47271696e6ad27a3d Mon Sep 17 00:00:00 2001 From: Vedran Pavic Date: Fri, 29 Apr 2016 20:53:21 +0200 Subject: [PATCH 1/2] Improve AuditEventRepository See gh-5854 --- .../boot/actuate/audit/AuditEvent.java | 3 +- .../actuate/audit/AuditEventRepository.java | 22 ++++- .../audit/InMemoryAuditEventRepository.java | 94 ++++++++++++++----- .../boot/actuate/audit/AuditEventTests.java | 32 ++++++- .../InMemoryAuditEventRepositoryTests.java | 30 +++++- 5 files changed, 152 insertions(+), 29 deletions(-) diff --git a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/audit/AuditEvent.java b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/audit/AuditEvent.java index 34f9d619df..d1b9089c1d 100644 --- a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/audit/AuditEvent.java +++ b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/audit/AuditEvent.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2015 the original author or authors. + * Copyright 2012-2016 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -80,6 +80,7 @@ public class AuditEvent implements Serializable { public AuditEvent(Date timestamp, String principal, String type, Map data) { Assert.notNull(timestamp, "Timestamp must not be null"); + Assert.notNull(principal, "Principal must not be null"); Assert.notNull(type, "Type must not be null"); this.timestamp = timestamp; this.principal = principal; diff --git a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/audit/AuditEventRepository.java b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/audit/AuditEventRepository.java index 0f966ddc6a..c6c9c3951f 100644 --- a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/audit/AuditEventRepository.java +++ b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/audit/AuditEventRepository.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2014 the original author or authors. + * Copyright 2012-2016 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -23,9 +23,18 @@ import java.util.List; * Repository for {@link AuditEvent}s. * * @author Dave Syer + * @author Vedran Pavic */ public interface AuditEventRepository { + /** + * Find audit events since the time provided. + * @param after timestamp of earliest result required + * @return audit events + * @since 1.4.0 + */ + List find(Date after); + /** * Find audit events relating to the specified principal since the time provided. * @param principal the principal name to search for @@ -34,6 +43,17 @@ public interface AuditEventRepository { */ List find(String principal, Date after); + /** + * Find audit events of specified type relating to the specified principal since the + * time provided. + * @param principal the principal name to search for + * @param type the event type to search for + * @param after timestamp of earliest result required + * @return audit events of specified type relating to the principal + * @since 1.4.0 + */ + List find(String principal, String type, Date after); + /** * Log an event. * @param event the audit event to log diff --git a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/audit/InMemoryAuditEventRepository.java b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/audit/InMemoryAuditEventRepository.java index 087a51f141..2fcefcf233 100644 --- a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/audit/InMemoryAuditEventRepository.java +++ b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/audit/InMemoryAuditEventRepository.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2015 the original author or authors. + * Copyright 2012-2016 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -20,11 +20,14 @@ import java.util.Date; import java.util.LinkedList; import java.util.List; +import org.springframework.util.Assert; + /** * In-memory {@link AuditEventRepository} implementation. * * @author Dave Syer * @author Phillip Webb + * @author Vedran Pavic */ public class InMemoryAuditEventRepository implements AuditEventRepository { @@ -33,7 +36,7 @@ public class InMemoryAuditEventRepository implements AuditEventRepository { /** * Circular buffer of the event with tail pointing to the last element. */ - private AuditEvent[] events; + private final AuditEvent[] events; private volatile int tail = -1; @@ -45,39 +48,84 @@ public class InMemoryAuditEventRepository implements AuditEventRepository { this.events = new AuditEvent[capacity]; } - /** - * Set the capacity of this event repository. - * @param capacity the capacity - */ - public synchronized void setCapacity(int capacity) { - this.events = new AuditEvent[capacity]; + @Override + public List find(Date after) { + LinkedList events = new LinkedList(); + synchronized (this.events) { + for (int i = 0; i < this.events.length; i++) { + AuditEvent event = resolveTailEvent(i); + if (event == null) { + break; + } + if (isMatch(event, after)) { + events.addFirst(event); + } + } + } + return events; } @Override - public synchronized List find(String principal, Date after) { + public List find(String principal, Date after) { + Assert.notNull(principal, "Principal must not be null"); LinkedList events = new LinkedList(); - for (int i = 0; i < this.events.length; i++) { - int index = ((this.tail + this.events.length - i) % this.events.length); - AuditEvent event = this.events[index]; - if (event == null) { - break; - } - if (isMatch(event, principal, after)) { - events.addFirst(event); + synchronized (this.events) { + for (int i = 0; i < this.events.length; i++) { + AuditEvent event = resolveTailEvent(i); + if (event == null) { + break; + } + if (isMatch(event, principal, after)) { + events.addFirst(event); + } } } return events; } - private boolean isMatch(AuditEvent auditEvent, String principal, Date after) { - return (principal == null || auditEvent.getPrincipal().equals(principal)) - && (after == null || auditEvent.getTimestamp().compareTo(after) >= 0); + @Override + public List find(String principal, String type, Date after) { + Assert.notNull(principal, "Principal must not be null"); + Assert.notNull(type, "Type must not be null"); + LinkedList events = new LinkedList(); + synchronized (this.events) { + for (int i = 0; i < this.events.length; i++) { + AuditEvent event = resolveTailEvent(i); + if (event == null) { + break; + } + if (isMatch(event, principal, type, after)) { + events.addFirst(event); + } + } + } + return events; } @Override - public synchronized void add(AuditEvent event) { - this.tail = (this.tail + 1) % this.events.length; - this.events[this.tail] = event; + public void add(AuditEvent event) { + Assert.notNull(event, "AuditEvent must not be null"); + synchronized (this.events) { + this.tail = (this.tail + 1) % this.events.length; + this.events[this.tail] = event; + } + } + + private AuditEvent resolveTailEvent(int offset) { + int index = ((this.tail + this.events.length - offset) % this.events.length); + return this.events[index]; + } + + private boolean isMatch(AuditEvent event, Date after) { + return (after == null || event.getTimestamp().compareTo(after) >= 0); + } + + private boolean isMatch(AuditEvent event, String principal, Date after) { + return (event.getPrincipal().equals(principal) && isMatch(event, after)); + } + + private boolean isMatch(AuditEvent event, String principal, String type, Date after) { + return (event.getType().equals(type) && isMatch(event, principal, after)); } } diff --git a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/audit/AuditEventTests.java b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/audit/AuditEventTests.java index d0f0b9c814..4313f5f304 100644 --- a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/audit/AuditEventTests.java +++ b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/audit/AuditEventTests.java @@ -18,7 +18,9 @@ package org.springframework.boot.actuate.audit; import java.util.Collections; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; import static org.assertj.core.api.Assertions.assertThat; @@ -26,11 +28,15 @@ import static org.assertj.core.api.Assertions.assertThat; * Tests for {@link AuditEvent}. * * @author Dave Syer + * @author Vedran Pavic */ public class AuditEventTests { + @Rule + public ExpectedException thrown = ExpectedException.none(); + @Test - public void testNowEvent() throws Exception { + public void nowEvent() throws Exception { AuditEvent event = new AuditEvent("phil", "UNKNOWN", Collections.singletonMap("a", (Object) "b")); assertThat(event.getData().get("a")).isEqualTo("b"); @@ -40,10 +46,32 @@ public class AuditEventTests { } @Test - public void testConvertStringsToData() throws Exception { + public void convertStringsToData() throws Exception { AuditEvent event = new AuditEvent("phil", "UNKNOWN", "a=b", "c=d"); assertThat(event.getData().get("a")).isEqualTo("b"); assertThat(event.getData().get("c")).isEqualTo("d"); } + @Test + public void nullTimestamp() throws Exception { + this.thrown.expect(IllegalArgumentException.class); + this.thrown.expectMessage("Timestamp must not be null"); + new AuditEvent(null, "phil", "UNKNOWN", + Collections.singletonMap("a", (Object) "b")); + } + + @Test + public void nullPrincipal() throws Exception { + this.thrown.expect(IllegalArgumentException.class); + this.thrown.expectMessage("Principal must not be null"); + new AuditEvent(null, "UNKNOWN", Collections.singletonMap("a", (Object) "b")); + } + + @Test + public void nullType() throws Exception { + this.thrown.expect(IllegalArgumentException.class); + this.thrown.expectMessage("Type must not be null"); + new AuditEvent("phil", null, Collections.singletonMap("a", (Object) "b")); + } + } diff --git a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/audit/InMemoryAuditEventRepositoryTests.java b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/audit/InMemoryAuditEventRepositoryTests.java index 90078e94c3..d010b31f94 100644 --- a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/audit/InMemoryAuditEventRepositoryTests.java +++ b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/audit/InMemoryAuditEventRepositoryTests.java @@ -22,7 +22,9 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; import static org.assertj.core.api.Assertions.assertThat; @@ -31,9 +33,13 @@ import static org.assertj.core.api.Assertions.assertThat; * * @author Dave Syer * @author Phillip Webb + * @author Vedran Pavic */ public class InMemoryAuditEventRepositoryTests { + @Rule + public ExpectedException thrown = ExpectedException.none(); + @Test public void lessThanCapacity() throws Exception { InMemoryAuditEventRepository repository = new InMemoryAuditEventRepository(); @@ -43,7 +49,6 @@ public class InMemoryAuditEventRepositoryTests { assertThat(events.size()).isEqualTo(2); assertThat(events.get(0).getType()).isEqualTo("a"); assertThat(events.get(1).getType()).isEqualTo("b"); - } @Test @@ -58,6 +63,14 @@ public class InMemoryAuditEventRepositoryTests { assertThat(events.get(1).getType()).isEqualTo("c"); } + @Test + public void addNullAuditEvent() throws Exception { + this.thrown.expect(IllegalArgumentException.class); + this.thrown.expectMessage("AuditEvent must not be null"); + InMemoryAuditEventRepository repository = new InMemoryAuditEventRepository(); + repository.add(null); + } + @Test public void findByPrincipal() throws Exception { InMemoryAuditEventRepository repository = new InMemoryAuditEventRepository(); @@ -71,6 +84,19 @@ public class InMemoryAuditEventRepositoryTests { assertThat(events.get(1).getType()).isEqualTo("c"); } + @Test + public void findByPrincipalAndType() throws Exception { + InMemoryAuditEventRepository repository = new InMemoryAuditEventRepository(); + repository.add(new AuditEvent("dave", "a")); + repository.add(new AuditEvent("phil", "b")); + repository.add(new AuditEvent("dave", "c")); + repository.add(new AuditEvent("phil", "d")); + List events = repository.find("dave", "a", null); + assertThat(events.size()).isEqualTo(1); + assertThat(events.get(0).getPrincipal()).isEqualTo("dave"); + assertThat(events.get(0).getType()).isEqualTo("a"); + } + @Test public void findByDate() throws Exception { Calendar calendar = Calendar.getInstance(); @@ -87,7 +113,7 @@ public class InMemoryAuditEventRepositoryTests { calendar.add(Calendar.DAY_OF_YEAR, 1); repository.add(new AuditEvent(calendar.getTime(), "phil", "d", data)); calendar.add(Calendar.DAY_OF_YEAR, 1); - List events = repository.find(null, after); + List events = repository.find(after); assertThat(events.size()).isEqualTo(2); assertThat(events.get(0).getType()).isEqualTo("c"); assertThat(events.get(1).getType()).isEqualTo("d"); From 3ba2b24301d149ef935d27416170cb2165792412 Mon Sep 17 00:00:00 2001 From: Stephane Nicoll Date: Thu, 30 Jun 2016 11:44:01 +0200 Subject: [PATCH 2/2] Polish "Improve AuditEventRepository" Closes gh-5854 --- .../boot/actuate/audit/AuditEvent.java | 4 +- .../actuate/audit/AuditEventRepository.java | 4 +- .../audit/InMemoryAuditEventRepository.java | 72 +++++++++---------- .../InMemoryAuditEventRepositoryTests.java | 2 +- 4 files changed, 41 insertions(+), 41 deletions(-) diff --git a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/audit/AuditEvent.java b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/audit/AuditEvent.java index d1b9089c1d..dbfeffaf14 100644 --- a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/audit/AuditEvent.java +++ b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/audit/AuditEvent.java @@ -111,8 +111,8 @@ public class AuditEvent implements Serializable { } /** - * Returns the user principal responsible for the event or {@code null}. - * @return the principal or {@code null} + * Returns the user principal responsible for the event. + * @return the principal */ public String getPrincipal() { return this.principal; diff --git a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/audit/AuditEventRepository.java b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/audit/AuditEventRepository.java index c6c9c3951f..746dc8d492 100644 --- a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/audit/AuditEventRepository.java +++ b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/audit/AuditEventRepository.java @@ -47,12 +47,12 @@ public interface AuditEventRepository { * Find audit events of specified type relating to the specified principal since the * time provided. * @param principal the principal name to search for - * @param type the event type to search for * @param after timestamp of earliest result required + * @param type the event type to search for * @return audit events of specified type relating to the principal * @since 1.4.0 */ - List find(String principal, String type, Date after); + List find(String principal, Date after, String type); /** * Log an event. diff --git a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/audit/InMemoryAuditEventRepository.java b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/audit/InMemoryAuditEventRepository.java index 2fcefcf233..5b730c5966 100644 --- a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/audit/InMemoryAuditEventRepository.java +++ b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/audit/InMemoryAuditEventRepository.java @@ -36,7 +36,7 @@ public class InMemoryAuditEventRepository implements AuditEventRepository { /** * Circular buffer of the event with tail pointing to the last element. */ - private final AuditEvent[] events; + private AuditEvent[] events; private volatile int tail = -1; @@ -48,67 +48,67 @@ public class InMemoryAuditEventRepository implements AuditEventRepository { this.events = new AuditEvent[capacity]; } + /** + * Set the capacity of this event repository. + * @param capacity the capacity + */ + public synchronized void setCapacity(int capacity) { + this.events = new AuditEvent[capacity]; + } + @Override - public List find(Date after) { + public synchronized List find(Date after) { LinkedList events = new LinkedList(); - synchronized (this.events) { - for (int i = 0; i < this.events.length; i++) { - AuditEvent event = resolveTailEvent(i); - if (event == null) { - break; - } - if (isMatch(event, after)) { - events.addFirst(event); - } + for (int i = 0; i < this.events.length; i++) { + AuditEvent event = resolveTailEvent(i); + if (event == null) { + break; + } + if (isMatch(event, after)) { + events.addFirst(event); } } return events; } @Override - public List find(String principal, Date after) { + public synchronized List find(String principal, Date after) { Assert.notNull(principal, "Principal must not be null"); LinkedList events = new LinkedList(); - synchronized (this.events) { - for (int i = 0; i < this.events.length; i++) { - AuditEvent event = resolveTailEvent(i); - if (event == null) { - break; - } - if (isMatch(event, principal, after)) { - events.addFirst(event); - } + for (int i = 0; i < this.events.length; i++) { + AuditEvent event = resolveTailEvent(i); + if (event == null) { + break; + } + if (isMatch(event, principal, after)) { + events.addFirst(event); } } return events; } @Override - public List find(String principal, String type, Date after) { + public synchronized List find(String principal, Date after, String type) { Assert.notNull(principal, "Principal must not be null"); Assert.notNull(type, "Type must not be null"); LinkedList events = new LinkedList(); - synchronized (this.events) { - for (int i = 0; i < this.events.length; i++) { - AuditEvent event = resolveTailEvent(i); - if (event == null) { - break; - } - if (isMatch(event, principal, type, after)) { - events.addFirst(event); - } + for (int i = 0; i < this.events.length; i++) { + AuditEvent event = resolveTailEvent(i); + if (event == null) { + break; + } + if (isMatch(event, principal, type, after)) { + events.addFirst(event); } } return events; } @Override - public void add(AuditEvent event) { + public synchronized void add(AuditEvent event) { Assert.notNull(event, "AuditEvent must not be null"); - synchronized (this.events) { - this.tail = (this.tail + 1) % this.events.length; - this.events[this.tail] = event; - } + this.tail = (this.tail + 1) % this.events.length; + this.events[this.tail] = event; } private AuditEvent resolveTailEvent(int offset) { diff --git a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/audit/InMemoryAuditEventRepositoryTests.java b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/audit/InMemoryAuditEventRepositoryTests.java index d010b31f94..a4aa8d298f 100644 --- a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/audit/InMemoryAuditEventRepositoryTests.java +++ b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/audit/InMemoryAuditEventRepositoryTests.java @@ -91,7 +91,7 @@ public class InMemoryAuditEventRepositoryTests { repository.add(new AuditEvent("phil", "b")); repository.add(new AuditEvent("dave", "c")); repository.add(new AuditEvent("phil", "d")); - List events = repository.find("dave", "a", null); + List events = repository.find("dave", null, "a"); assertThat(events.size()).isEqualTo(1); assertThat(events.get(0).getPrincipal()).isEqualTo("dave"); assertThat(events.get(0).getType()).isEqualTo("a");