From ba42d6b4946f866ab184e03473b52c177923ae04 Mon Sep 17 00:00:00 2001 From: Joseph Liechty Date: Tue, 28 Apr 2026 11:41:15 -0500 Subject: [PATCH] refactor: remove unnecessary lock, fix deprecated API, guard null in serializer HstRequestContext is per-request/thread-local so the synchronized block in GenericResourceEntityBuilder.get() was dead weight with no safety benefit. IOUtils.closeQuietly was deprecated since commons-io 2.5 and removed in 2.11+, replaced with try-with-resources. HstLinkSerializer could NPE if a null HstLink was passed to serialize(). Tests added for all three changes. (co-authored with Claude Code) --- .../GenericResourceEntityBuilder.java | 12 +----- ...GenericResourceEntityAggregationValve.java | 9 +--- .../jackson/HstLinkSerializer.java | 5 +++ .../GenericResourceEntityBuilderTest.java | 17 ++++++++ .../jackson/HstLinkSerializerTest.java | 41 +++++++++++++++++++ 5 files changed, 67 insertions(+), 17 deletions(-) create mode 100644 src/test/java/com/onehippo/cms7/genericresource/entitybuilder/jackson/HstLinkSerializerTest.java diff --git a/src/main/java/com/onehippo/cms7/genericresource/entitybuilder/GenericResourceEntityBuilder.java b/src/main/java/com/onehippo/cms7/genericresource/entitybuilder/GenericResourceEntityBuilder.java index 6b11bab..0a33275 100644 --- a/src/main/java/com/onehippo/cms7/genericresource/entitybuilder/GenericResourceEntityBuilder.java +++ b/src/main/java/com/onehippo/cms7/genericresource/entitybuilder/GenericResourceEntityBuilder.java @@ -32,20 +32,12 @@ public abstract class GenericResourceEntityBuilder { private static final String NAME = GenericResourceEntityBuilder.class.getName(); - private static final Object lock = new Object(); - public static GenericResourceEntityBuilder get(final HstRequestContext requestContext) { GenericResourceEntityBuilder builder = (GenericResourceEntityBuilder) requestContext.getAttribute(NAME); if (builder == null) { - synchronized (lock) { - builder = (GenericResourceEntityBuilder) requestContext.getAttribute(NAME); - - if (builder == null) { - builder = new DefaultGenericResourceEntityBuilder(); - requestContext.setAttribute(NAME, builder); - } - } + builder = new DefaultGenericResourceEntityBuilder(); + requestContext.setAttribute(NAME, builder); } return builder; diff --git a/src/main/java/com/onehippo/cms7/genericresource/entitybuilder/container/GenericResourceEntityAggregationValve.java b/src/main/java/com/onehippo/cms7/genericresource/entitybuilder/container/GenericResourceEntityAggregationValve.java index 964cb97..c6393cf 100644 --- a/src/main/java/com/onehippo/cms7/genericresource/entitybuilder/container/GenericResourceEntityAggregationValve.java +++ b/src/main/java/com/onehippo/cms7/genericresource/entitybuilder/container/GenericResourceEntityAggregationValve.java @@ -21,7 +21,6 @@ import jakarta.servlet.http.HttpServletResponse; -import org.apache.commons.io.IOUtils; import org.apache.commons.lang3.StringUtils; import org.apache.commons.lang3.Strings; import org.hippoecm.hst.container.RequestContextProvider; @@ -89,18 +88,14 @@ protected void processWindowsRender(final HstContainerConfig requestContainerCon GenericResourceEntityBuilder builder = GenericResourceEntityBuilder.get(requestContext); - PrintWriter writer = null; + response.setContentType("application/json"); - try { - response.setContentType("application/json"); - writer = response.getWriter(); + try (PrintWriter writer = response.getWriter()) { builder.write(getObjectMapper(), writer); } catch (GenericResourceEntityBuilderException e) { log.warn("Failed to generate json.", e); } catch (IOException e) { log.warn("Failed to write json.", e); - } finally { - IOUtils.closeQuietly(writer); } } } diff --git a/src/main/java/com/onehippo/cms7/genericresource/entitybuilder/jackson/HstLinkSerializer.java b/src/main/java/com/onehippo/cms7/genericresource/entitybuilder/jackson/HstLinkSerializer.java index b759db9..e5921f1 100644 --- a/src/main/java/com/onehippo/cms7/genericresource/entitybuilder/jackson/HstLinkSerializer.java +++ b/src/main/java/com/onehippo/cms7/genericresource/entitybuilder/jackson/HstLinkSerializer.java @@ -39,6 +39,11 @@ public HstLinkSerializer(Class type) { @Override public void serialize(HstLink value, JsonGenerator gen, SerializerProvider provider) throws IOException { + if (value == null) { + gen.writeNull(); + return; + } + final HstRequestContext requestContext = RequestContextProvider.get(); gen.writeStartObject(); diff --git a/src/test/java/com/onehippo/cms7/genericresource/entitybuilder/GenericResourceEntityBuilderTest.java b/src/test/java/com/onehippo/cms7/genericresource/entitybuilder/GenericResourceEntityBuilderTest.java index 90c41b8..653efc0 100644 --- a/src/test/java/com/onehippo/cms7/genericresource/entitybuilder/GenericResourceEntityBuilderTest.java +++ b/src/test/java/com/onehippo/cms7/genericresource/entitybuilder/GenericResourceEntityBuilderTest.java @@ -16,6 +16,9 @@ import org.junit.Test; import org.onehippo.repository.mock.MockNode; +import static org.junit.Assert.assertNotSame; +import static org.junit.Assert.assertSame; + import com.fasterxml.jackson.databind.ObjectMapper; import com.onehippo.cms7.genericresource.entitybuilder.jackson.DefaultJsonIgnoreTypeMixin; import com.onehippo.cms7.genericresource.entitybuilder.jackson.HstBeansExcludingObjectMapperDecorator; @@ -37,6 +40,20 @@ public void setUp() throws Exception { objectMapper = decorator.decorate(new ObjectMapper()); } + @Test + public void get_returnsSameInstanceForSameRequestContext() { + GenericResourceEntityBuilder first = GenericResourceEntityBuilder.get(requestContext); + GenericResourceEntityBuilder second = GenericResourceEntityBuilder.get(requestContext); + assertSame(first, second); + } + + @Test + public void get_returnsNewInstanceForDifferentRequestContext() { + GenericResourceEntityBuilder first = GenericResourceEntityBuilder.get(requestContext); + GenericResourceEntityBuilder second = GenericResourceEntityBuilder.get(new MockHstRequestContext()); + assertNotSame(first, second); + } + @Test public void testJsonOut() throws Exception { GenericResourceEntityBuilder builder = GenericResourceEntityBuilder.get(requestContext); diff --git a/src/test/java/com/onehippo/cms7/genericresource/entitybuilder/jackson/HstLinkSerializerTest.java b/src/test/java/com/onehippo/cms7/genericresource/entitybuilder/jackson/HstLinkSerializerTest.java new file mode 100644 index 0000000..78be841 --- /dev/null +++ b/src/test/java/com/onehippo/cms7/genericresource/entitybuilder/jackson/HstLinkSerializerTest.java @@ -0,0 +1,41 @@ +/* + * Copyright 2017-2025 BloomReach, Inc. (https://www.bloomreach.com) + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.onehippo.cms7.genericresource.entitybuilder.jackson; + +import com.fasterxml.jackson.core.JsonGenerator; +import com.fasterxml.jackson.databind.SerializerProvider; +import org.junit.Test; + +import static org.easymock.EasyMock.createMock; +import static org.easymock.EasyMock.replay; +import static org.easymock.EasyMock.verify; + +public class HstLinkSerializerTest { + + @Test + public void serialize_nullValue_writesJsonNull() throws Exception { + JsonGenerator gen = createMock(JsonGenerator.class); + SerializerProvider provider = createMock(SerializerProvider.class); + + gen.writeNull(); + + replay(gen, provider); + + new HstLinkSerializer().serialize(null, gen, provider); + + verify(gen, provider); + } +}