From 9f3e44aeb73c5cc5e6d8019aa00899b728cf2d74 Mon Sep 17 00:00:00 2001 From: querwurzel <> Date: Sat, 14 Dec 2024 12:31:28 +0100 Subject: [PATCH] fix: improve domain, enhance load test, more optionals --- .../paste/application/PasteViewService.java | 16 +--- .../github/binpastes/paste/domain/Paste.java | 43 ++++++----- .../binpastes/paste/domain/PasteService.java | 8 +- .../binpastes/paste/OneTimePasteIT.java | 17 +++-- .../github/binpastes/paste/PublicPasteIT.java | 4 +- .../github/binpastes/paste/SearchPasteIT.java | 2 +- .../binpastes/paste/TrackingPasteIT.java | 8 +- .../binpastes/paste/UnlistedPasteIT.java | 4 +- .../binpastes/paste/domain/PasteTest.java | 73 +++++++++---------- 9 files changed, 86 insertions(+), 89 deletions(-) diff --git a/backend/src/main/java/com/github/binpastes/paste/application/PasteViewService.java b/backend/src/main/java/com/github/binpastes/paste/application/PasteViewService.java index 1411d5a..bd55afc 100644 --- a/backend/src/main/java/com/github/binpastes/paste/application/PasteViewService.java +++ b/backend/src/main/java/com/github/binpastes/paste/application/PasteViewService.java @@ -47,21 +47,7 @@ public Mono viewPaste(String id, String remoteAddress) { public Mono viewOneTimePaste(String id) { return pasteService.findAndBurn(id) - .map(paste -> new DetailView( - paste.getId(), - paste.getTitle(), - paste.getContent(), - paste.getContent().getBytes().length, - paste.isPublic(), - false, // paste just burnt - paste.isEncrypted(), - paste.isOneTime(), - paste.isPermanent(), - paste.getDateCreated(), - paste.getDateOfExpiry(), - paste.getLastViewed(), - paste.getViews() - )); + .map(paste -> DetailView.of(paste, null)); } public Mono viewAllPastes() { diff --git a/backend/src/main/java/com/github/binpastes/paste/domain/Paste.java b/backend/src/main/java/com/github/binpastes/paste/domain/Paste.java index 032cf07..16d44ca 100644 --- a/backend/src/main/java/com/github/binpastes/paste/domain/Paste.java +++ b/backend/src/main/java/com/github/binpastes/paste/domain/Paste.java @@ -13,6 +13,7 @@ import static com.github.binpastes.paste.domain.Paste.PasteSchema; import static java.util.Objects.isNull; +import static java.util.Objects.nonNull; @Table(PasteSchema.TABLE_NAME) public class Paste { @@ -50,19 +51,23 @@ public class Paste { private Long views; public static Paste newInstance( - String title, - String content, - LocalDateTime dateOfExpiry, - boolean isEncrypted, - PasteExposure exposure, - String remoteAddress + final String title, + final String content, + final boolean isEncrypted, + final PasteExposure exposure, + final LocalDateTime dateOfExpiry, + final String remoteAddress ) { + if (nonNull(dateOfExpiry) && LocalDateTime.now().isAfter(dateOfExpiry)) { + throw new IllegalArgumentException("dateOfExpiry must be in the future!"); + } + return new Paste() + .setContent(Objects.requireNonNull(content)) + .setExposure(Objects.requireNonNull(exposure)) .setId(IdGenerator.newStringId()) .setTitle(title) - .setContent(Objects.requireNonNull(content)) .setIsEncrypted(isEncrypted) - .setExposure(Objects.requireNonNull(exposure)) .setRemoteAddress(remoteAddress) .setDateOfExpiry(dateOfExpiry) .setViews(0); @@ -92,8 +97,8 @@ private PasteExposure getExposure() { return exposure; } - private String getRemoteAddress() { - return remoteAddress; + private Optional getRemoteAddress() { + return Optional.ofNullable(remoteAddress); } public Optional getLastViewed() { @@ -125,12 +130,12 @@ public boolean isPermanent() { } public boolean isErasable(String remoteAddress) { - if (isUnlisted() || isOneTime()) { + if (isUnlisted() || (isOneTime() && !isExpired())) { return true; } if (isPublic()) { - final var createdBySameAuthor = Objects.equals(remoteAddress, getRemoteAddress()); + final var createdBySameAuthor = Objects.equals(remoteAddress, getRemoteAddress().orElse(null)); if (createdBySameAuthor) { return LocalDateTime.now().minusHours(1).isBefore(getDateCreated()); @@ -149,17 +154,17 @@ public Paste trackView(LocalDateTime lastViewed) { return setViews(getViews() + 1); } - public Paste markAsExpired() { - return markAsExpired(LocalDateTime.now()); + public boolean isExpired() { + var currentExpiry = getDateOfExpiry(); + return currentExpiry.isPresent() && currentExpiry.get().isBefore(LocalDateTime.now()); } - public Paste markAsExpired(LocalDateTime dateOfExpiry) { - var currentExpiry = getDateOfExpiry(); - if (currentExpiry.isEmpty() || dateOfExpiry.isBefore(currentExpiry.get())) { - return setDateOfExpiry(dateOfExpiry); + public Paste markAsExpired() { + if (isExpired()) { + return this; } - throw new IllegalStateException("Paste has already expired: " + currentExpiry.get()); + return setDateOfExpiry(LocalDateTime.now()); } protected Paste setId(final String id) { diff --git a/backend/src/main/java/com/github/binpastes/paste/domain/PasteService.java b/backend/src/main/java/com/github/binpastes/paste/domain/PasteService.java index 5775fc7..52ab5f5 100644 --- a/backend/src/main/java/com/github/binpastes/paste/domain/PasteService.java +++ b/backend/src/main/java/com/github/binpastes/paste/domain/PasteService.java @@ -36,7 +36,7 @@ public Mono create( String remoteAddress ) { return pasteRepository - .save(Paste.newInstance(title, content, dateOfExpiry, isEncrypted, exposure, remoteAddress)) + .save(Paste.newInstance(title, content, isEncrypted, exposure, dateOfExpiry, remoteAddress)) .doOnSuccess(newPaste -> log.info("Created new paste {}", newPaste.getId())) .doOnError(throwable -> log.error("Failed to create new paste", throwable)); } @@ -53,7 +53,8 @@ public Mono findAndBurn(String id) { .filter(Paste::isOneTime) .map(Paste::markAsExpired) .flatMap(pasteRepository::save) - .doOnNext(paste -> log.info("OneTime paste {} viewed and burnt", paste.getId())); + .doOnNext(paste -> log.info("OneTime paste {} viewed and burnt", paste.getId())) + .onErrorComplete(OptimisticLockingFailureException.class); } public Flux findAll() { @@ -78,11 +79,10 @@ public void trackView(String id, LocalDateTime viewedAt) { } public Mono requestDeletion(String id, String remoteAddress) { - var now = LocalDateTime.now(); return pasteRepository .findOneLegitById(id) .filter(paste -> paste.isErasable(remoteAddress)) - .map(paste -> paste.markAsExpired(now)) + .map(Paste::markAsExpired) .flatMap(pasteRepository::save) .retryWhen(Retry .backoff(5, Duration.ofMillis(500)) diff --git a/backend/src/test/java/com/github/binpastes/paste/OneTimePasteIT.java b/backend/src/test/java/com/github/binpastes/paste/OneTimePasteIT.java index df46540..8b9bce0 100644 --- a/backend/src/test/java/com/github/binpastes/paste/OneTimePasteIT.java +++ b/backend/src/test/java/com/github/binpastes/paste/OneTimePasteIT.java @@ -69,8 +69,9 @@ void getOneTimePasteHidesContent() { void viewOneTimePasteConcurrently() { var oneTimePaste = givenOneTimePaste(); var okCount = new AtomicInteger(); + var notFoundCount = new AtomicInteger(); - final Runnable call = () -> { + final Runnable call = new Thread(() -> { try { webClient.post() .uri("/api/v1/paste/{id}", oneTimePaste.getId()) @@ -91,15 +92,21 @@ void viewOneTimePasteConcurrently() { )); okCount.incrementAndGet(); - } catch (AssertionError ignored) {} - }; + } catch (AssertionError ex) { + if (ex.getMessage().contains("404")) { + notFoundCount.incrementAndGet(); + } + } + }); Stream.generate(() -> call) - .parallel() .limit(100) + .toList() + .parallelStream() .forEach(Runnable::run); assertThat(okCount.get()).isOne(); + assertThat(notFoundCount.get()).isEqualTo(100 - 1); } @Test @@ -193,9 +200,9 @@ private Paste givenOneTimePaste() { Paste.newInstance( "someTitle", "Lorem ipsum dolor sit amet", - null, false, PasteExposure.ONCE, + null, "1.1.1.1" ) ); diff --git a/backend/src/test/java/com/github/binpastes/paste/PublicPasteIT.java b/backend/src/test/java/com/github/binpastes/paste/PublicPasteIT.java index 1ea60e1..6dcfd8f 100644 --- a/backend/src/test/java/com/github/binpastes/paste/PublicPasteIT.java +++ b/backend/src/test/java/com/github/binpastes/paste/PublicPasteIT.java @@ -59,9 +59,9 @@ void getExpiringPublicPaste() { var paste = givenPaste(Paste.newInstance( "someTitle", "Lorem ipsum dolor sit amet", - LocalDateTime.now().plusMinutes(1).minusSeconds(3), // expiry before max-age false, PasteExposure.PUBLIC, + LocalDateTime.now().plusMinutes(1).minusSeconds(3), // expiry before max-age "1.1.1.1" )); @@ -185,9 +185,9 @@ private Paste givenPublicPaste() { Paste.newInstance( "someTitle", "Lorem ipsum dolor sit amet", - null, false, PasteExposure.PUBLIC, + null, "someRemoteAddress" ) ); diff --git a/backend/src/test/java/com/github/binpastes/paste/SearchPasteIT.java b/backend/src/test/java/com/github/binpastes/paste/SearchPasteIT.java index 09accb7..6b54405 100644 --- a/backend/src/test/java/com/github/binpastes/paste/SearchPasteIT.java +++ b/backend/src/test/java/com/github/binpastes/paste/SearchPasteIT.java @@ -65,9 +65,9 @@ private Paste givenPublicPaste() { Paste.newInstance( "someTitle", "Lorem ipsum dolor sit amet", - null, false, Paste.PasteExposure.PUBLIC, + null, "someRemoteAddress" ) ); diff --git a/backend/src/test/java/com/github/binpastes/paste/TrackingPasteIT.java b/backend/src/test/java/com/github/binpastes/paste/TrackingPasteIT.java index 7d8cc04..1901e4b 100644 --- a/backend/src/test/java/com/github/binpastes/paste/TrackingPasteIT.java +++ b/backend/src/test/java/com/github/binpastes/paste/TrackingPasteIT.java @@ -81,12 +81,12 @@ void trackConcurrentPasteViews() { Flux.fromStream(Stream.generate(intialPaste::getId)) .take(concurrency) .doOnNext(trackingService::trackView) - // simulate a concurrent update .doOnNext(id -> pasteRepository.findById(id) .doOnNext(paste -> setField(paste, "remoteAddress", String.valueOf(random.nextInt()))) - .flatMap(paste -> pasteRepository.save(paste)) + .flatMap(paste -> pasteRepository.save(paste)) // simulate a concurrent update .retry() - .subscribe()) + .subscribe() + ) .subscribeOn(Schedulers.parallel()) .subscribe(); @@ -114,9 +114,9 @@ private Paste givenPublicPaste() { Paste.newInstance( "someTitle", "Lorem ipsum dolor sit amet", - null, false, PasteExposure.PUBLIC, + null, "someRemoteAddress" ) ); diff --git a/backend/src/test/java/com/github/binpastes/paste/UnlistedPasteIT.java b/backend/src/test/java/com/github/binpastes/paste/UnlistedPasteIT.java index 67daa22..334d04a 100644 --- a/backend/src/test/java/com/github/binpastes/paste/UnlistedPasteIT.java +++ b/backend/src/test/java/com/github/binpastes/paste/UnlistedPasteIT.java @@ -60,9 +60,9 @@ void getExpiringUnlistedPaste() { var unlistedPaste = givenPaste(Paste.newInstance( "someTitle", "Lorem ipsum dolor sit amet", - LocalDateTime.now().plusMinutes(1).minusSeconds(3), // expiry before max-age false, PasteExposure.UNLISTED, + LocalDateTime.now().plusMinutes(1).minusSeconds(3), // expiry before max-age "1.1.1.1" )); @@ -165,9 +165,9 @@ private Paste givenUnlistedPaste() { Paste.newInstance( "someTitle", "Lorem ipsum dolor sit amet", - null, false, PasteExposure.UNLISTED, + null, "1.1.1.1" ) ); diff --git a/backend/src/test/java/com/github/binpastes/paste/domain/PasteTest.java b/backend/src/test/java/com/github/binpastes/paste/domain/PasteTest.java index de48263..a483922 100644 --- a/backend/src/test/java/com/github/binpastes/paste/domain/PasteTest.java +++ b/backend/src/test/java/com/github/binpastes/paste/domain/PasteTest.java @@ -24,21 +24,27 @@ class PasteTest { void newInstanceMandatoryFields() { assertThrows( NullPointerException.class, - () -> Paste.newInstance(null, null, null, false, PasteExposure.PUBLIC, null), + () -> Paste.newInstance(null, null, false, PasteExposure.PUBLIC, null, null), "content is mandatory" ); assertThrows( NullPointerException.class, - () -> Paste.newInstance(null, "someContent", null, false, null, null), + () -> Paste.newInstance(null, "someContent", false, null, null, null), "exposure is mandatory" ); + + assertThrows( + IllegalArgumentException.class, + () -> Paste.newInstance(null, "someContent", false, null, LocalDateTime.now(), null), + "expiry must be in the future" + ); } @Test @DisplayName("new paste - default values are set") void newInstanceDefaults() { - var newPaste = Paste.newInstance(null, "someContent", null, false, PasteExposure.PUBLIC, null); + var newPaste = Paste.newInstance(null, "someContent", false, PasteExposure.PUBLIC, null, null); assertThat(newPaste.getId()).isNotEmpty(); assertThat(newPaste.getViews()).isZero(); @@ -48,7 +54,7 @@ void newInstanceDefaults() { @Test @DisplayName("track paste - increases view count") void trackPasteViewCount() { - var newPaste = Paste.newInstance(null, "someContent", null, false, PasteExposure.PUBLIC, null); + var newPaste = Paste.newInstance(null, "someContent", false, PasteExposure.PUBLIC, null, null); newPaste.trackView(LocalDateTime.now()); newPaste.trackView(LocalDateTime.now()); @@ -59,7 +65,7 @@ void trackPasteViewCount() { @Test @DisplayName("track paste - updates lastViewed timestamp to most recent one") void trackPasteLastViewed() { - var newPaste = Paste.newInstance(null, "someContent", null, false, PasteExposure.PUBLIC, null); + var newPaste = Paste.newInstance(null, "someContent", false, PasteExposure.PUBLIC, null, null); var today = LocalDateTime.now(); var yesterday = today.minusDays(1); @@ -71,33 +77,23 @@ void trackPasteLastViewed() { } @ParameterizedTest - @DisplayName("markAsExpired - sets date of expiry to current timestamp") + @DisplayName("markAsExpired - set date of expiry to timestamp if not already expired") @MethodSource("datesOfExpiry") - void markAsExpired(LocalDateTime dateOfExpiry, boolean exceptionExpected) { - var paste = Paste.newInstance( - "someTitle", - "someContent", - dateOfExpiry, - false, - PasteExposure.PUBLIC, - null); - - if (exceptionExpected) { - assertThrows(IllegalStateException.class, paste::markAsExpired); - assertThat(paste.getDateOfExpiry()).hasValue(dateOfExpiry); - } else { - paste.markAsExpired(); - - assertThat(paste.getDateOfExpiry().get()) - .isCloseTo(LocalDateTime.now(), new TemporalUnitWithinOffset(59, ChronoUnit.SECONDS)); - } + void markAsExpired(LocalDateTime dateOfExpiry, LocalDateTime expectedExpiry) { + var paste = Paste.newInstance(null, "someContent", false, PasteExposure.PUBLIC, null, null); + paste.setDateOfExpiry(dateOfExpiry); + + paste.markAsExpired(); + + assertThat(paste.getDateOfExpiry().get()) + .isCloseTo(expectedExpiry, new TemporalUnitWithinOffset(3, ChronoUnit.SECONDS)); } private static Stream datesOfExpiry() { return Stream.of( - arguments(named("permanent paste that never expires", null), false), - arguments(named("paste expires in the future", LocalDateTime.now().plusDays(1)), false), - arguments(named("paste already expired", LocalDateTime.now().minusDays(1)), true) + arguments(named("permanent paste that never expires", null), LocalDateTime.now()), + arguments(named("paste expires in the future", LocalDateTime.now().plusDays(1)), LocalDateTime.now()), + arguments(named("paste already expired", LocalDateTime.now().minusDays(1)), LocalDateTime.now().minusDays(1)) ); } @@ -110,24 +106,27 @@ void isErasable(Paste paste, String requestedBy, boolean erasable) { } private static Stream pastesToErase() { - var unlistedPaste = Paste.newInstance(null, "someContent", null, false, PasteExposure.UNLISTED, "Alice"); - var oneTimePaste = Paste.newInstance(null, "someContent", null, false, PasteExposure.ONCE, "Alice"); + var unlistedPaste = Paste.newInstance(null, "someContent", false, PasteExposure.UNLISTED, null, "Alice"); + var oneTimePaste = Paste.newInstance(null, "someContent", false, PasteExposure.ONCE, null, "Alice"); + var oneTimePasteExpired = Paste.newInstance(null, "someContent", false, PasteExposure.ONCE, null, "Alice"); + oneTimePasteExpired.setDateOfExpiry(LocalDateTime.now().minusHours(1)); - var publicPasteRecentlyCreated = Paste.newInstance(null, "someContent", null, false, PasteExposure.PUBLIC, "Alice"); - publicPasteRecentlyCreated.setDateCreated(LocalDateTime.now().minusMinutes(60).plusSeconds(1)); + var publicPasteCreatedLastHour = Paste.newInstance(null, "someContent", false, PasteExposure.PUBLIC, null, "Alice"); + publicPasteCreatedLastHour.setDateCreated(LocalDateTime.now().minusMinutes(60).plusSeconds(1)); - var publicPasteCreatedSomeTimeAgo = Paste.newInstance(null, "someContent", null, false, PasteExposure.PUBLIC, "Alice"); - publicPasteCreatedSomeTimeAgo.setDateCreated(LocalDateTime.now().minusMinutes(60)); + var publicPasteCreatedAnHourAgo = Paste.newInstance(null, "someContent", false, PasteExposure.PUBLIC, null, "Alice"); + publicPasteCreatedAnHourAgo.setDateCreated(LocalDateTime.now().minusMinutes(60)); return Stream.of( arguments(named("unlisted paste", unlistedPaste), null, true), arguments(named("one-time paste", oneTimePaste), null, true), + arguments(named("one-time paste expired", oneTimePasteExpired), null, false), - arguments(named("public paste with recent dateCreated", publicPasteRecentlyCreated), "Alice", true), - arguments(named("public paste with recent dateCreated requested by foreigner", publicPasteRecentlyCreated), "Bob", false), + arguments(named("public paste created last hour", publicPasteCreatedLastHour), "Alice", true), + arguments(named("public paste created last hour requested by foreigner", publicPasteCreatedLastHour), "Bob", false), - arguments(named("public paste with older dateCreated", publicPasteCreatedSomeTimeAgo), "Alice", false), - arguments(named("public paste with older dateCreated requested by foreigner", publicPasteCreatedSomeTimeAgo), "Bob", false) + arguments(named("public paste created one hour ago", publicPasteCreatedAnHourAgo), "Alice", false), + arguments(named("public paste created one hour ago requested by foreigner", publicPasteCreatedAnHourAgo), "Bob", false) ); } }