From ba96118b4ea5a39b928e4d68d8a9c3dec0f1b570 Mon Sep 17 00:00:00 2001 From: John Niang Date: Mon, 3 Jun 2024 10:42:13 +0800 Subject: [PATCH] Fix the problem that bundle files can be generated arbitrarily (#6028) #### What type of PR is this? /kind bug /area core /area plugin /milestone 2.16.0 #### What this PR does / why we need it: Before the PR, any user can generate bundle files by providing random query param `v` while requesting bundle files. This PR refactors the whole bundle file generation method. 1. Do nothing if users provide arbitrary bundle file version 2. Better lock for writing bundle files if not exist #### Special notes for your reviewer: 1. Request `http://localhost:8090/apis/api.console.halo.run/v1alpha1/plugins/-/bundle.js?v=xyz` 2. Check if the file `xyz.js` in folder `$TMPDIR/halo-plugin-bundle**` #### Does this PR introduce a user-facing change? ```release-note None ``` --- .../extension/endpoint/PluginEndpoint.java | 194 +++--------------- .../core/extension/service/PluginService.java | 43 +++- .../service/impl/PluginServiceImpl.java | 169 +++++++++++++-- .../endpoint/PluginEndpointTest.java | 107 +--------- .../service/impl/PluginServiceImplTest.java | 137 ++++++++++++- 5 files changed, 364 insertions(+), 286 deletions(-) diff --git a/application/src/main/java/run/halo/app/core/extension/endpoint/PluginEndpoint.java b/application/src/main/java/run/halo/app/core/extension/endpoint/PluginEndpoint.java index c796a8b4b6..e295d2f925 100644 --- a/application/src/main/java/run/halo/app/core/extension/endpoint/PluginEndpoint.java +++ b/application/src/main/java/run/halo/app/core/extension/endpoint/PluginEndpoint.java @@ -32,25 +32,16 @@ import java.util.List; import java.util.Map; import java.util.Objects; -import java.util.Optional; -import java.util.concurrent.atomic.AtomicReference; -import java.util.concurrent.locks.ReadWriteLock; -import java.util.concurrent.locks.ReentrantReadWriteLock; import java.util.function.Function; import java.util.function.Predicate; -import java.util.function.Supplier; import lombok.Data; import lombok.extern.slf4j.Slf4j; import org.reactivestreams.Publisher; import org.springdoc.core.fn.builders.operation.Builder; import org.springdoc.webflux.core.fn.SpringdocRouteBuilder; -import org.springframework.beans.factory.DisposableBean; import org.springframework.beans.factory.InitializingBean; import org.springframework.boot.autoconfigure.web.WebProperties; -import org.springframework.core.io.FileSystemResource; -import org.springframework.core.io.Resource; import org.springframework.core.io.buffer.DataBuffer; -import org.springframework.core.io.buffer.DataBufferUtils; import org.springframework.dao.OptimisticLockingFailureException; import org.springframework.data.domain.Sort; import org.springframework.http.CacheControl; @@ -58,10 +49,7 @@ import org.springframework.http.codec.multipart.FilePart; import org.springframework.http.codec.multipart.FormFieldPart; import org.springframework.http.codec.multipart.Part; -import org.springframework.lang.Nullable; import org.springframework.stereotype.Component; -import org.springframework.util.Assert; -import org.springframework.util.FileSystemUtils; import org.springframework.util.MultiValueMap; import org.springframework.util.StringUtils; import org.springframework.web.reactive.function.BodyInserters; @@ -71,7 +59,6 @@ import org.springframework.web.reactive.resource.NoResourceFoundException; import org.springframework.web.server.ServerWebExchange; import org.springframework.web.server.ServerWebInputException; -import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; import reactor.core.scheduler.Scheduler; import reactor.core.scheduler.Schedulers; @@ -98,8 +85,6 @@ public class PluginEndpoint implements CustomEndpoint, InitializingBean { private final ReactiveUrlDataBufferFetcher reactiveUrlDataBufferFetcher; - private final BufferedPluginBundleResource bufferedPluginBundleResource; - private final WebProperties webProperties; private final Scheduler scheduler = Schedulers.boundedElastic(); @@ -111,12 +96,10 @@ public class PluginEndpoint implements CustomEndpoint, InitializingBean { public PluginEndpoint(ReactiveExtensionClient client, PluginService pluginService, ReactiveUrlDataBufferFetcher reactiveUrlDataBufferFetcher, - BufferedPluginBundleResource bufferedPluginBundleResource, WebProperties webProperties) { this.client = client; this.pluginService = pluginService; this.reactiveUrlDataBufferFetcher = reactiveUrlDataBufferFetcher; - this.bufferedPluginBundleResource = bufferedPluginBundleResource; this.webProperties = webProperties; } @@ -326,52 +309,38 @@ static class RunningStateRequest { } private Mono fetchJsBundle(ServerRequest request) { - Optional versionOption = request.queryParam("v"); - if (versionOption.isEmpty()) { - return pluginService.generateJsBundleVersion() + var versionOption = request.queryParam("v"); + return versionOption.map(s -> pluginService.getJsBundle(s).flatMap( + jsRes -> { + var bodyBuilder = ServerResponse.ok() + .cacheControl(bundleCacheControl) + .contentType(MediaType.valueOf("text/javascript")); + if (useLastModified) { + try { + var lastModified = Instant.ofEpochMilli(jsRes.lastModified()); + bodyBuilder = bodyBuilder.lastModified(lastModified); + } catch (IOException e) { + if (e instanceof FileNotFoundException) { + return Mono.error(new NoResourceFoundException("bundle.js")); + } + return Mono.error(e); + } + } + return bodyBuilder.body(BodyInserters.fromResource(jsRes)); + })) + .orElseGet(() -> pluginService.generateBundleVersion() .flatMap(version -> ServerResponse .temporaryRedirect(buildJsBundleUri("js", version)) .cacheControl(CacheControl.noStore()) - .build()); - } - var version = versionOption.get(); - return bufferedPluginBundleResource.getJsBundle(version, pluginService::uglifyJsBundle) - .flatMap(jsRes -> { - var bodyBuilder = ServerResponse.ok() - .cacheControl(bundleCacheControl) - .contentType(MediaType.valueOf("text/javascript")); - if (useLastModified) { - try { - var lastModified = Instant.ofEpochMilli(jsRes.lastModified()); - bodyBuilder = bodyBuilder.lastModified(lastModified); - } catch (IOException e) { - if (e instanceof FileNotFoundException) { - return Mono.error(new NoResourceFoundException("bundle.js")); - } - return Mono.error(e); - } - } - return bodyBuilder.body(BodyInserters.fromResource(jsRes)); - }); + .build())); } private Mono fetchCssBundle(ServerRequest request) { - Optional versionOption = request.queryParam("v"); - if (versionOption.isEmpty()) { - return pluginService.generateJsBundleVersion() - .flatMap(version -> ServerResponse - .temporaryRedirect(buildJsBundleUri("css", version)) - .cacheControl(CacheControl.noStore()) - .build()); - } - - var version = versionOption.get(); - return bufferedPluginBundleResource.getCssBundle(version, pluginService::uglifyCssBundle) - .flatMap(cssRes -> { + return request.queryParam("v") + .map(s -> pluginService.getCssBundle(s).flatMap(cssRes -> { var bodyBuilder = ServerResponse.ok() .cacheControl(bundleCacheControl) .contentType(MediaType.valueOf("text/css")); - if (useLastModified) { try { var lastModified = Instant.ofEpochMilli(cssRes.lastModified()); @@ -383,9 +352,14 @@ private Mono fetchCssBundle(ServerRequest request) { return Mono.error(e); } } - return bodyBuilder.body(BodyInserters.fromResource(cssRes)); - }); + })) + .orElseGet(() -> pluginService.generateBundleVersion() + .flatMap(version -> ServerResponse + .temporaryRedirect(buildJsBundleUri("css", version)) + .cacheControl(CacheControl.noStore()) + .build())); + } URI buildJsBundleUri(String type, String version) { @@ -765,112 +739,4 @@ private Mono writeToTempFile(Publisher content) { .subscribeOn(this.scheduler); } - @Component - static class BufferedPluginBundleResource implements DisposableBean { - - private final AtomicReference jsBundle = new AtomicReference<>(); - private final AtomicReference cssBundle = new AtomicReference<>(); - - private final ReadWriteLock jsLock = new ReentrantReadWriteLock(); - private final ReadWriteLock cssLock = new ReentrantReadWriteLock(); - - private Path tempDir; - - public Mono getJsBundle(String version, - Supplier> jsSupplier) { - var fileName = tempFileName(version, ".js"); - return Mono.defer(() -> { - jsLock.readLock().lock(); - try { - var jsBundleResource = jsBundle.get(); - if (getResourceIfNotChange(fileName, jsBundleResource) != null) { - return Mono.just(jsBundleResource); - } - } finally { - jsLock.readLock().unlock(); - } - - jsLock.writeLock().lock(); - try { - var oldJsBundle = jsBundle.get(); - return writeBundle(fileName, jsSupplier) - .doOnNext(newRes -> jsBundle.compareAndSet(oldJsBundle, newRes)); - } finally { - jsLock.writeLock().unlock(); - } - }).subscribeOn(Schedulers.boundedElastic()); - } - - public Mono getCssBundle(String version, - Supplier> cssSupplier) { - var fileName = tempFileName(version, ".css"); - return Mono.defer(() -> { - cssLock.readLock().lock(); - try { - var cssBundleResource = cssBundle.get(); - if (getResourceIfNotChange(fileName, cssBundleResource) != null) { - return Mono.just(cssBundleResource); - } - } finally { - cssLock.readLock().unlock(); - } - - cssLock.writeLock().lock(); - try { - var oldCssBundle = cssBundle.get(); - return writeBundle(fileName, cssSupplier) - .doOnNext(newRes -> cssBundle.compareAndSet(oldCssBundle, newRes)); - } finally { - cssLock.writeLock().unlock(); - } - }).subscribeOn(Schedulers.boundedElastic()); - } - - @Nullable - private Resource getResourceIfNotChange(String fileName, Resource resource) { - if (resource != null && resource.exists() && fileName.equals(resource.getFilename())) { - return resource; - } - return null; - } - - private Mono writeBundle(String fileName, - Supplier> dataSupplier) { - return Mono.defer( - () -> { - var filePath = createTempFileToStore(fileName); - return DataBufferUtils.write(dataSupplier.get(), filePath) - .then(Mono.fromSupplier(() -> new FileSystemResource(filePath))); - }); - } - - private Path createTempFileToStore(String fileName) { - try { - if (tempDir == null || !Files.exists(tempDir)) { - this.tempDir = Files.createTempDirectory("halo-plugin-bundle"); - } - var path = tempDir.resolve(fileName); - Files.deleteIfExists(path); - return Files.createFile(path); - } catch (IOException e) { - throw new ServerWebInputException("Failed to create temp file.", null, e); - } - } - - private String tempFileName(String v, String suffix) { - Assert.notNull(v, "Version must not be null"); - Assert.notNull(suffix, "Suffix must not be null"); - return v + suffix; - } - - @Override - public void destroy() throws Exception { - if (tempDir != null && Files.exists(tempDir)) { - FileSystemUtils.deleteRecursively(tempDir); - } - this.jsBundle.set(null); - this.cssBundle.set(null); - } - } - } diff --git a/application/src/main/java/run/halo/app/core/extension/service/PluginService.java b/application/src/main/java/run/halo/app/core/extension/service/PluginService.java index 7fe693e6ca..7c86444899 100644 --- a/application/src/main/java/run/halo/app/core/extension/service/PluginService.java +++ b/application/src/main/java/run/halo/app/core/extension/service/PluginService.java @@ -1,6 +1,7 @@ package run.halo.app.core.extension.service; import java.nio.file.Path; +import org.springframework.core.io.Resource; import org.springframework.core.io.buffer.DataBuffer; import org.springframework.web.server.ServerWebInputException; import reactor.core.publisher.Flux; @@ -56,12 +57,48 @@ public interface PluginService { Flux uglifyCssBundle(); /** - *

Generate js bundle version for cache control.

+ *

Generate js/css bundle version for cache control.

* This method will list all enabled plugins version and sign it to a string. * - * @return signed js bundle version by all enabled plugins version. + * @return signed js/css bundle version by all enabled plugins version. */ - Mono generateJsBundleVersion(); + Mono generateBundleVersion(); + + /** + * Retrieves the JavaScript bundle for all enabled plugins. + * + *

This method combines the JavaScript bundles of all enabled plugins into a single bundle + * and returns a representation of this bundle as a resource. + * If the JavaScript bundle already exists and is up-to-date, the existing resource is + * returned; otherwise, a new JavaScript bundle is generated. + * + *

Note: This method may perform IO operations and could potentially block, so it should be + * used in a non-blocking environment. + * + * @param version The version of the CSS bundle to retrieve. + * @return A {@code Mono} object representing the JavaScript bundle. When this + * {@code Mono} is subscribed to, it emits the JavaScript bundle resource if successful, or + * an error signal if an error occurs. + */ + Mono getJsBundle(String version); + + /** + * Retrieves the CSS bundle for all enabled plugins. + * + *

This method combines the CSS bundles of all enabled plugins into a single bundle and + * returns a representation of this bundle as a resource. + * If the CSS bundle already exists and is up-to-date, the existing resource is returned; + * otherwise, a new CSS bundle is generated. + * + *

Note: This method may perform IO operations and could potentially block, so it should be + * used in a non-blocking environment. + * + * @param version The version of the CSS bundle to retrieve. + * @return A {@code Mono} object representing the CSS bundle. When this {@code Mono + * } is subscribed to, it emits the CSS bundle resource if successful, or an error signal if + * an error occurs. + */ + Mono getCssBundle(String version); /** * Enables or disables a plugin by name. diff --git a/application/src/main/java/run/halo/app/core/extension/service/impl/PluginServiceImpl.java b/application/src/main/java/run/halo/app/core/extension/service/impl/PluginServiceImpl.java index 8becfeb845..89ec3e1d57 100644 --- a/application/src/main/java/run/halo/app/core/extension/service/impl/PluginServiceImpl.java +++ b/application/src/main/java/run/halo/app/core/extension/service/impl/PluginServiceImpl.java @@ -1,10 +1,13 @@ package run.halo.app.core.extension.service.impl; import static java.nio.file.StandardCopyOption.REPLACE_EXISTING; +import static java.nio.file.StandardOpenOption.CREATE; +import static java.nio.file.StandardOpenOption.TRUNCATE_EXISTING; import static org.pf4j.PluginState.STARTED; import static run.halo.app.plugin.PluginConst.RELOAD_ANNO; import com.github.zafarkhaja.semver.Version; +import com.google.common.hash.HashCode; import com.google.common.hash.Hashing; import java.io.IOException; import java.nio.charset.StandardCharsets; @@ -17,13 +20,18 @@ import java.util.HashMap; import java.util.List; import java.util.Objects; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.stream.Collectors; -import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.pf4j.DependencyResolver; import org.pf4j.PluginDescriptor; import org.pf4j.PluginWrapper; +import org.reactivestreams.Publisher; +import org.springframework.beans.factory.DisposableBean; +import org.springframework.beans.factory.InitializingBean; +import org.springframework.core.io.FileSystemResource; import org.springframework.core.io.Resource; +import org.springframework.core.io.WritableResource; import org.springframework.core.io.buffer.DataBuffer; import org.springframework.core.io.buffer.DataBufferUtils; import org.springframework.core.io.buffer.DefaultDataBufferFactory; @@ -31,10 +39,12 @@ import org.springframework.stereotype.Component; import org.springframework.util.Assert; import org.springframework.util.CollectionUtils; +import org.springframework.util.FileSystemUtils; import org.springframework.web.server.ServerWebInputException; import reactor.core.Exceptions; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; +import reactor.core.scheduler.Scheduler; import reactor.core.scheduler.Schedulers; import reactor.util.retry.Retry; import run.halo.app.core.extension.Plugin; @@ -59,8 +69,7 @@ @Slf4j @Component -@RequiredArgsConstructor -public class PluginServiceImpl implements PluginService { +public class PluginServiceImpl implements PluginService, InitializingBean, DisposableBean { private static final String PRESET_LOCATION_PREFIX = "classpath:/presets/plugins/"; private static final String PRESETS_LOCATION_PATTERN = PRESET_LOCATION_PREFIX + "*.jar"; @@ -73,6 +82,25 @@ public class PluginServiceImpl implements PluginService { private final SpringPluginManager pluginManager; + private final BundleCache jsBundleCache; + + private final BundleCache cssBundleCache; + + private Path tempDir; + + private final Scheduler scheduler = Schedulers.boundedElastic(); + + public PluginServiceImpl(ReactiveExtensionClient client, SystemVersionSupplier systemVersion, + PluginProperties pluginProperties, SpringPluginManager pluginManager) { + this.client = client; + this.systemVersion = systemVersion; + this.pluginProperties = pluginProperties; + this.pluginManager = pluginManager; + + this.jsBundleCache = new BundleCache(".js"); + this.cssBundleCache = new BundleCache(".css"); + } + @Override public Flux getPresets() { // list presets from classpath @@ -240,17 +268,23 @@ public Flux uglifyCssBundle() { } @Override - public Mono generateJsBundleVersion() { - return Mono.fromSupplier(() -> { - var compactVersion = pluginManager.getStartedPlugins() - .stream() - .sorted(Comparator.comparing(PluginWrapper::getPluginId)) - .map(pluginWrapper -> pluginWrapper.getPluginId() + ":" - + pluginWrapper.getDescriptor().getVersion() - ) - .collect(Collectors.joining()); - return Hashing.sha256().hashUnencodedChars(compactVersion).toString(); - }); + public Mono generateBundleVersion() { + return Flux.fromIterable(new ArrayList<>(pluginManager.getStartedPlugins())) + .sort(Comparator.comparing(PluginWrapper::getPluginId)) + .map(pw -> pw.getPluginId() + ':' + pw.getDescriptor().getVersion()) + .collect(Collectors.joining()) + .map(Hashing.sha256()::hashUnencodedChars) + .map(HashCode::toString); + } + + @Override + public Mono getJsBundle(String version) { + return jsBundleCache.computeIfAbsent(version, this.uglifyJsBundle()); + } + + @Override + public Mono getCssBundle(String version) { + return cssBundleCache.computeIfAbsent(version, this.uglifyCssBundle()); } @Override @@ -344,6 +378,26 @@ Mono findPluginManifest(Path path) { ); } + + @Override + public void afterPropertiesSet() throws Exception { + this.tempDir = Files.createTempDirectory("halo-plugin-bundle"); + } + + @Override + public void destroy() throws Exception { + FileSystemUtils.deleteRecursively(this.tempDir); + } + + /** + * Set temporary directory for plugin bundle. + * + * @param tempDir temporary directory. + */ + void setTempDir(Path tempDir) { + this.tempDir = tempDir; + } + /** * Copy plugin into plugin home. * @@ -450,8 +504,93 @@ private static void updatePlugin(Plugin oldPlugin, Plugin newPlugin) { oldPlugin.getSpec().setEnabled(enabled); } - private static class UnexpectedPluginStateException extends RuntimeException { + class BundleCache { + private final String suffix; + + private final AtomicBoolean writing = new AtomicBoolean(); + + private volatile Resource resource; + + BundleCache(String suffix) { + this.suffix = suffix; + } + + Mono computeIfAbsent(String version, Publisher content) { + var filename = buildBundleFilename(version, suffix); + if (isResourceMatch(resource, filename)) { + return Mono.just(resource); + } + return generateBundleVersion() + .flatMap(newVersion -> { + var newFilename = buildBundleFilename(newVersion, suffix); + if (isResourceMatch(this.resource, newFilename)) { + // if the resource was not changed, just return it + return Mono.just(resource); + } + if (writing.compareAndSet(false, true)) { + return Mono.justOrEmpty(this.resource) + // double check of the resource + .filter(res -> isResourceMatch(res, newFilename)) + .switchIfEmpty(Mono.using( + () -> tempDir.resolve(newFilename), + path -> DataBufferUtils.write(content, path, + CREATE, TRUNCATE_EXISTING) + .then(Mono.fromSupplier( + () -> new FileSystemResource(path) + )), + path -> { + // clean up old resource + cleanUp(this.resource); + }) + .subscribeOn(scheduler) + .doOnNext(newResource -> this.resource = newResource) + ) + .doFinally(signalType -> writing.set(false)); + } else { + return Mono.defer(() -> { + if (this.writing.get()) { + log.debug("Waiting for the bundle file {} to be written", filename); + return Mono.empty(); + } + log.debug("Waited the bundle file {} to be written", filename); + return Mono.just(this.resource); + }).repeatWhenEmpty(100, count -> { + // retry after 100ms + return count.delayElements(Duration.ofMillis(100)); + }); + } + }); + } + + private static void cleanUp(Resource resource) { + if (resource instanceof WritableResource wr + && wr.isWritable() + && wr.isFile()) { + try { + Files.deleteIfExists(wr.getFile().toPath()); + } catch (IOException e) { + log.warn("Failed to delete old bundle file {}", + wr.getFilename(), e); + } + } + } + + private static boolean isResourceMatch(Resource resource, String filename) { + return resource != null + && resource.exists() + && resource.isFile() + && Objects.equals(filename, resource.getFilename()); + } + } + + private static String buildBundleFilename(String v, String suffix) { + Assert.notNull(v, "Version must not be null"); + Assert.notNull(suffix, "Suffix must not be null"); + return v + suffix; + } + + private static class UnexpectedPluginStateException extends RuntimeException { } } diff --git a/application/src/test/java/run/halo/app/core/extension/endpoint/PluginEndpointTest.java b/application/src/test/java/run/halo/app/core/extension/endpoint/PluginEndpointTest.java index ae98a4809e..5bfe03569e 100644 --- a/application/src/test/java/run/halo/app/core/extension/endpoint/PluginEndpointTest.java +++ b/application/src/test/java/run/halo/app/core/extension/endpoint/PluginEndpointTest.java @@ -1,7 +1,6 @@ package run.halo.app.core.extension.endpoint; import static java.util.Objects.requireNonNull; -import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.argThat; @@ -41,19 +40,14 @@ import org.springframework.core.io.ByteArrayResource; import org.springframework.core.io.FileSystemResource; import org.springframework.core.io.Resource; -import org.springframework.core.io.buffer.DataBuffer; -import org.springframework.core.io.buffer.DefaultDataBufferFactory; import org.springframework.http.CacheControl; import org.springframework.http.MediaType; import org.springframework.http.client.MultipartBodyBuilder; import org.springframework.test.web.reactive.server.WebTestClient; import org.springframework.web.server.ServerWebInputException; -import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; -import reactor.test.StepVerifier; import run.halo.app.core.extension.Plugin; import run.halo.app.core.extension.Setting; -import run.halo.app.core.extension.endpoint.PluginEndpoint.BufferedPluginBundleResource; import run.halo.app.core.extension.service.PluginService; import run.halo.app.extension.ConfigMap; import run.halo.app.extension.ListResult; @@ -82,9 +76,6 @@ class PluginEndpointTest { @Spy WebProperties webProperties = new WebProperties(); - @Mock - BufferedPluginBundleResource bufferedPluginBundleResource; - @InjectMocks PluginEndpoint endpoint; @@ -403,92 +394,6 @@ Plugin createPlugin(String name, Instant creationTimestamp) { return plugin; } - @Nested - class BufferedPluginBundleResourceTest { - private final BufferedPluginBundleResource bufferedPluginBundleResource = - new BufferedPluginBundleResource(); - - private static Flux getDataBufferFlux(String x) { - var buffer = DefaultDataBufferFactory.sharedInstance - .wrap(x.getBytes(StandardCharsets.UTF_8)); - return Flux.just(buffer); - } - - @Test - void writeAndGetJsResourceTest() { - bufferedPluginBundleResource.getJsBundle("1", - () -> getDataBufferFlux("first line\nnext line")) - .as(StepVerifier::create) - .consumeNextWith(resource -> { - try { - String content = resource.getContentAsString(StandardCharsets.UTF_8); - assertThat(content).isEqualTo("first line\nnext line"); - } catch (IOException e) { - throw new RuntimeException(e); - } - }) - .verifyComplete(); - - // version is matched, should return cached content - bufferedPluginBundleResource.getJsBundle("1", - () -> getDataBufferFlux("first line\nnext line-1")) - .as(StepVerifier::create) - .consumeNextWith(resource -> { - try { - String content = resource.getContentAsString(StandardCharsets.UTF_8); - assertThat(content).isEqualTo("first line\nnext line"); - } catch (IOException e) { - throw new RuntimeException(e); - } - }) - .verifyComplete(); - - // new version should return new content - bufferedPluginBundleResource.getJsBundle("2", - () -> getDataBufferFlux("first line\nnext line-2")) - .as(StepVerifier::create) - .consumeNextWith(resource -> { - try { - String content = resource.getContentAsString(StandardCharsets.UTF_8); - assertThat(content).isEqualTo("first line\nnext line-2"); - } catch (IOException e) { - throw new RuntimeException(e); - } - }) - .verifyComplete(); - } - - @Test - void writeAndGetCssResourceTest() { - bufferedPluginBundleResource.getCssBundle("1", - () -> getDataBufferFlux("first line\nnext line")) - .as(StepVerifier::create) - .consumeNextWith(resource -> { - try { - String content = resource.getContentAsString(StandardCharsets.UTF_8); - assertThat(content).isEqualTo("first line\nnext line"); - } catch (IOException e) { - throw new RuntimeException(e); - } - }) - .verifyComplete(); - - // version is matched, should return cached content - bufferedPluginBundleResource.getCssBundle("1", - () -> getDataBufferFlux("first line\nnext line-1")) - .as(StepVerifier::create) - .consumeNextWith(resource -> { - try { - String content = resource.getContentAsString(StandardCharsets.UTF_8); - assertThat(content).isEqualTo("first line\nnext line"); - } catch (IOException e) { - throw new RuntimeException(e); - } - }) - .verifyComplete(); - } - } - @Nested class BundleResourceEndpointTest { @@ -507,7 +412,7 @@ void setUp() { @Test void shouldBeRedirectedWhileFetchingBundleJsWithoutVersion() { - when(pluginService.generateJsBundleVersion()).thenReturn(Mono.just("fake-version")); + when(pluginService.generateBundleVersion()).thenReturn(Mono.just("fake-version")); webClient.get().uri("/plugins/-/bundle.js") .exchange() .expectStatus().is3xxRedirection() @@ -518,7 +423,7 @@ void shouldBeRedirectedWhileFetchingBundleJsWithoutVersion() { @Test void shouldBeRedirectedWhileFetchingBundleCssWithoutVersion() { - when(pluginService.generateJsBundleVersion()).thenReturn(Mono.just("fake-version")); + when(pluginService.generateBundleVersion()).thenReturn(Mono.just("fake-version")); webClient.get().uri("/plugins/-/bundle.css") .exchange() .expectStatus().is3xxRedirection() @@ -535,7 +440,7 @@ void shouldFetchBundleCssWithCacheControl() { cachecontrol.setNoCache(true); endpoint.afterPropertiesSet(); - when(bufferedPluginBundleResource.getCssBundle(eq("fake-version"), any())) + when(pluginService.getCssBundle("fake-version")) .thenReturn(Mono.fromSupplier(() -> mockResource("fake-css"))); webClient.get().uri("/plugins/-/bundle.css?v=fake-version") .exchange() @@ -554,7 +459,7 @@ void shouldFetchBundleJsWithCacheControl() { cachecontrol.setNoStore(true); endpoint.afterPropertiesSet(); - when(bufferedPluginBundleResource.getJsBundle(eq("fake-version"), any())) + when(pluginService.getJsBundle("fake-version")) .thenReturn(Mono.fromSupplier(() -> mockResource("fake-js"))); webClient.get().uri("/plugins/-/bundle.js?v=fake-version") .exchange() @@ -567,7 +472,7 @@ void shouldFetchBundleJsWithCacheControl() { @Test void shouldFetchBundleCss() { - when(bufferedPluginBundleResource.getCssBundle(eq("fake-version"), any())) + when(pluginService.getCssBundle("fake-version")) .thenReturn(Mono.fromSupplier(() -> mockResource("fake-css"))); webClient.get().uri("/plugins/-/bundle.css?v=fake-version") .exchange() @@ -580,7 +485,7 @@ void shouldFetchBundleCss() { @Test void shouldFetchBundleJs() { - when(bufferedPluginBundleResource.getJsBundle(eq("fake-version"), any())) + when(pluginService.getJsBundle("fake-version")) .thenReturn(Mono.fromSupplier(() -> mockResource("fake-js"))); webClient.get().uri("/plugins/-/bundle.js?v=fake-version") .exchange() diff --git a/application/src/test/java/run/halo/app/core/extension/service/impl/PluginServiceImplTest.java b/application/src/test/java/run/halo/app/core/extension/service/impl/PluginServiceImplTest.java index aaa9e40523..15dfb2cb01 100644 --- a/application/src/test/java/run/halo/app/core/extension/service/impl/PluginServiceImplTest.java +++ b/application/src/test/java/run/halo/app/core/extension/service/impl/PluginServiceImplTest.java @@ -1,5 +1,6 @@ package run.halo.app.core.extension.service.impl; +import static java.nio.charset.StandardCharsets.UTF_8; import static java.util.Objects.requireNonNull; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -7,33 +8,45 @@ import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.isA; +import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import static org.springframework.core.io.buffer.DefaultDataBufferFactory.sharedInstance; import com.github.zafarkhaja.semver.Version; import com.google.common.hash.Hashing; import java.io.IOException; import java.net.URISyntaxException; +import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; +import java.util.ArrayList; import java.util.List; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; import java.util.function.Consumer; +import java.util.stream.IntStream; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.RepeatedTest; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.junit.jupiter.api.io.TempDir; import org.mockito.InjectMocks; import org.mockito.Mock; +import org.mockito.Spy; import org.mockito.junit.jupiter.MockitoExtension; import org.pf4j.PluginDescriptor; import org.pf4j.PluginWrapper; +import org.springframework.core.io.buffer.DataBuffer; import org.springframework.web.server.ServerWebInputException; import reactor.core.publisher.Mono; import reactor.test.StepVerifier; +import reactor.test.publisher.PublisherProbe; import run.halo.app.core.extension.Plugin; import run.halo.app.extension.Metadata; import run.halo.app.extension.ReactiveExtensionClient; @@ -60,6 +73,7 @@ class PluginServiceImplTest { @Mock SpringPluginManager pluginManager; + @Spy @InjectMocks PluginServiceImpl pluginService; @@ -237,7 +251,7 @@ void shouldReloadIfLoadLocationReady() { @Test - void generateJsBundleVersionTest() { + void generateBundleVersionTest() { var plugin1 = mock(PluginWrapper.class); var plugin2 = mock(PluginWrapper.class); var plugin3 = mock(PluginWrapper.class); @@ -262,7 +276,7 @@ void generateJsBundleVersionTest() { var result = Hashing.sha256().hashUnencodedChars(str).toString(); assertThat(result.length()).isEqualTo(64); - pluginService.generateJsBundleVersion() + pluginService.generateBundleVersion() .as(StepVerifier::create) .consumeNextWith(version -> assertThat(version).isEqualTo(result)) .verifyComplete(); @@ -275,7 +289,7 @@ void generateJsBundleVersionTest() { var str2 = "fake-1:1.0.0fake-2:2.0.0fake-4:3.0.0"; var result2 = Hashing.sha256().hashUnencodedChars(str2).toString(); when(pluginManager.getStartedPlugins()).thenReturn(List.of(plugin1, plugin2, plugin4)); - pluginService.generateJsBundleVersion() + pluginService.generateBundleVersion() .as(StepVerifier::create) .consumeNextWith(version -> assertThat(version).isEqualTo(result2)) .verifyComplete(); @@ -330,6 +344,123 @@ void shouldDisablePluginIfAlreadyStarted() { } } + @Nested + class BundleCacheTest { + + PluginServiceImpl.BundleCache cache; + + @TempDir + Path tempDir; + + @BeforeEach + void setUp() { + pluginService.setTempDir(tempDir); + cache = pluginService.new BundleCache(".js"); + } + + @Test + void shouldComputeBundleFileIfAbsent() { + doReturn(Mono.just("different-version")).when(pluginService).generateBundleVersion(); + var fakeContent = Mono.just(sharedInstance.wrap("fake-content".getBytes( + UTF_8))); + cache.computeIfAbsent("fake-version", fakeContent) + .as(StepVerifier::create) + .assertNext(resource -> { + try { + assertEquals(tempDir.resolve("different-version.js"), + resource.getFile().toPath()); + assertEquals("different-version.js", resource.getFilename()); + assertEquals("fake-content", resource.getContentAsString(UTF_8)); + } catch (IOException e) { + throw new RuntimeException(e); + } + }) + .verifyComplete(); + } + + @Test + void shouldNotComputeBundleFileIfPresentAndVersionIsMatch() { + shouldComputeBundleFileIfAbsent(); + + var fakeContent = Mono.just( + sharedInstance.wrap("another-fake-content".getBytes(UTF_8))); + + cache.computeIfAbsent("different-version", fakeContent) + .as(StepVerifier::create) + .assertNext(resource -> { + try { + assertEquals("different-version.js", resource.getFilename()); + // The content won't be changed if the version is matched. + assertEquals("fake-content", resource.getContentAsString(UTF_8)); + } catch (IOException e) { + throw new RuntimeException(e); + } + }) + .verifyComplete(); + } + + @Test + void shouldComputeBundleFileIfPresentButVersionMismatch() { + shouldComputeBundleFileIfAbsent(); + + var fakeContent = Mono.just( + sharedInstance.wrap("another-fake-content".getBytes(UTF_8))); + + doReturn(Mono.just("updated-version")).when(pluginService).generateBundleVersion(); + + cache.computeIfAbsent("mismatch-version", fakeContent) + .as(StepVerifier::create) + .assertNext(resource -> { + try { + assertTrue(Files.notExists(tempDir.resolve("different-version.js"))); + assertEquals("updated-version.js", resource.getFilename()); + assertEquals("another-fake-content", resource.getContentAsString(UTF_8)); + } catch (IOException e) { + throw new RuntimeException(e); + } + }) + .verifyComplete(); + } + + @RepeatedTest(10) + void concurrentComputeBundleFileIfAbsent() { + lenient().doReturn(Mono.just("different-version")) + .when(pluginService) + .generateBundleVersion(); + + var executorService = Executors.newCachedThreadPool(); + + var probes = new ArrayList>(); + List> futures = IntStream.range(0, 10) + .mapToObj(i -> executorService.submit(() -> { + var fakeContent = Mono.just(sharedInstance.wrap( + ("fake-content-" + i).getBytes(UTF_8) + )); + var probe = PublisherProbe.of(fakeContent); + probes.add(probe); + cache.computeIfAbsent("fake-version", probe.mono()) + .as(StepVerifier::create) + .expectNextCount(1) + .verifyComplete(); + })) + .toList(); + executorService.shutdown(); + futures.forEach(future -> { + try { + future.get(); + } catch (InterruptedException | ExecutionException e) { + throw new RuntimeException(e); + } + }); + + // ensure only one probe was subscribed + var subscribedCount = probes.stream() + .filter(PublisherProbe::wasSubscribed) + .count(); + assertEquals(1, subscribedCount); + } + } + Plugin createPlugin(String name, Consumer pluginConsumer) { var plugin = new Plugin(); plugin.setMetadata(new Metadata());