Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SLING-12636: Resource Resolver: add test coverage for background init #136

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

SLING-12636: Resource Resolver: add test coverage for background init #136

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4df109b
SLING-12636: Resource Resolver: add test coverage for background init
reschke Jan 23, 2025
ea037cd
SLING-12636: Resource Resolver: add test coverage for background init…
reschke Jan 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 52 additions & 19 deletions src/test/java/org/apache/sling/resourceresolver/impl/mapping/VanityPathMapEntriesTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,23 +90,26 @@ public class VanityPathMapEntriesTest extends AbstractMappingMapEntriesTest {
@Mock
private EventAdmin eventAdmin;

private int pageSize;
private boolean isMaxCachedVanityPathEntriesStartup;
private final int pageSize;
private final boolean isMaxCachedVanityPathEntriesStartup;
private final boolean isVanityPathCacheInitInBackground;

private int prevPageSize = 1000;

@Parameters(name="{0} {1}")
@Parameters(name="{0} {1} {2}")
public static Collection<Object[]> data() {
return List.of(new Object[][] {
{1000, false},
{1, true},
{1000, true}}
{1000, false, true},
{1, true, false},
{1000, true, false}}
);
}

public VanityPathMapEntriesTest(int pageSize, boolean isMaxCachedVanityPathEntriesStartup) {
public VanityPathMapEntriesTest(int pageSize, boolean isMaxCachedVanityPathEntriesStartup,
boolean isVanityPathCacheInitInBackground) {
this.pageSize = pageSize;
this.isMaxCachedVanityPathEntriesStartup = isMaxCachedVanityPathEntriesStartup;
this.isVanityPathCacheInitInBackground = isVanityPathCacheInitInBackground;
}

@Override
Expand All @@ -133,13 +136,15 @@ public void setup() throws Exception {
Collections.sort(configs);
when(bundle.getSymbolicName()).thenReturn("TESTBUNDLE");
when(bundleContext.getBundle()).thenReturn(bundle);
when(resourceResolverFactory.getServiceUserAuthenticationInfo(anyString())).thenReturn(Map.of());
when(resourceResolverFactory.getServiceResourceResolver(any(Map.class))).thenReturn(resourceResolver);
when(resourceResolverFactory.isVanityPathEnabled()).thenReturn(true);
when(resourceResolverFactory.getVanityPathConfig()).thenReturn(configs);
when(resourceResolverFactory.getObservationPaths()).thenReturn(new Path[] {new Path("/")});
when(resourceResolverFactory.getMapRoot()).thenReturn(MapEntries.DEFAULT_MAP_ROOT);
when(resourceResolverFactory.getMaxCachedVanityPathEntries()).thenReturn(-1L);
when(resourceResolverFactory.isMaxCachedVanityPathEntriesStartup()).thenReturn(isMaxCachedVanityPathEntriesStartup);
when(resourceResolverFactory.isVanityPathCacheInitInBackground()).thenReturn(isVanityPathCacheInitInBackground);
when(resourceResolver.findResources(anyString(), eq("sql"))).thenReturn(
Collections.emptyIterator());
when(resourceResolver.findResources(anyString(), eq("JCR-SQL2"))).thenReturn(
Expand All @@ -149,6 +154,36 @@ public void setup() throws Exception {
Optional<ResourceResolverMetrics> metrics = Optional.empty();

mapEntries = new MapEntries(resourceResolverFactory, bundleContext, eventAdmin, stringInterpolationProvider, metrics);
waitForBgInit();
}

// get internal flag that signals completion of background task
private AtomicBoolean getVanityPathsProcessed() {
try {
Field field = MapEntries.class.getDeclaredField("vanityPathsProcessed");
field.setAccessible(true);
return (AtomicBoolean) field.get(mapEntries);
} catch (Exception e) {
throw new RuntimeException(e);
}
}

// wait for background thread to complete
private void waitForBgInit() {
while (!getVanityPathsProcessed().get()) {
try {
Thread.sleep(10);
} catch (InterruptedException e) {
// ignored
}
}
}

// get vanity paths (after waiting for bg init to complete)
private void initializeVanityPaths() throws IOException {
getVanityPathsProcessed().set(false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need to reset the internal state here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to check whether background init has finished; otherwise the expections about the number of entries in the map will be incorrect.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't follow the full logic; you are replacing doInit with changing an internal flag; this IMO makes the test assume too much about the behaviour of MapEntries and is going to make evolving this class more difficult.

Have you considered adding a new protected/package-private method that will encapsulate the re-reading of vanity paths?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll do that in later PRs.

This change is focused on improving test coverage without refactoring MapEntries. Tests will be updated when refactoring is done.

FWIW, doinit() actually initializes aliases, not vanity paths. All of this will be improved step by step.

Copy link
Contributor Author

@reschke reschke Jan 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need to reset the internal state here?

I checked, and it is needed. I currently do not understand what sets the state to "true", but I'll try to find out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. The reason is that the internal state of MapEntries did not account for loadVanityPaths called twice.

I added that, and was able to remove getVanityPathsProcessed().set(false); from the test.

Your point about extending MapEntries is understood; however the point of this change was to improve test coverage before doing any changes of what's being tested.

mapEntries.initializeVanityPaths();
waitForBgInit();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do I get it right, that you added the waitForBgInit() just in preparation for the next change, where you will run tests also with isVanityPathCacheInitInBackground = true?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests (now) run with bginit as well, see parameters of the test class.

}

@Override
Expand All @@ -166,8 +201,9 @@ public void test_simple_vanity_path() throws IOException {
String oneMore = "one-more";
prepareMapEntriesForVanityPath(false, false, containerName,
childName, oneMore, vanityPath);
mapEntries.doInit();
mapEntries.initializeVanityPaths();

initializeVanityPaths();

Map<String, List<String>> vanityMap = mapEntries.getVanityPathMappings();
assertNotNull(vanityMap);
assertEquals(vanityPath, vanityMap.get("/" + containerName + "/" + childName).get(0));
Expand All @@ -184,8 +220,9 @@ public void test_simple_vanity_path_support_with_null_parent() throws IOExceptio
String oneMore = "one-more";
prepareMapEntriesForVanityPath(true, true, containerName,
childName, oneMore, vanityPath);
mapEntries.doInit();
mapEntries.initializeVanityPaths();

initializeVanityPaths();

Map<String, List<String>> vanityMap = mapEntries.getVanityPathMappings();
assertNotNull(vanityMap);
// not present
Expand Down Expand Up @@ -299,8 +336,7 @@ public void test_vanity_path_registration() throws Exception {
}
});

mapEntries.doInit();
mapEntries.initializeVanityPaths();
initializeVanityPaths();

List<MapEntry> entries = mapEntries.getResolveMaps();

Expand Down Expand Up @@ -346,8 +382,7 @@ public void test_vanity_path_updates() throws Exception {

when(resourceResolver.findResources(anyString(), eq("JCR-SQL2"))).thenAnswer((Answer<Iterator<Resource>>) invocation -> Collections.emptyIterator());

mapEntries.doInit();
mapEntries.initializeVanityPaths();
initializeVanityPaths();

// map entries should have no alias atm
assertTrue( mapEntries.getResolveMaps().isEmpty());
Expand Down Expand Up @@ -460,7 +495,6 @@ public void test_vanity_path_registration_include_exclude() throws IOException {
resources.add(getVanityPathResource(val));
}


when(resourceResolver.findResources(anyString(), eq("JCR-SQL2"))).thenAnswer((Answer<Iterator<Resource>>) invocation -> {
String query = invocation.getArguments()[0].toString();
if (matchesPagedQuery(query)) {
Expand All @@ -475,8 +509,7 @@ public void test_vanity_path_registration_include_exclude() throws IOException {
}
});

mapEntries.doInit();
mapEntries.initializeVanityPaths();
initializeVanityPaths();

List<MapEntry> entries = mapEntries.getResolveMaps();
// each valid resource results in 2 entries
Expand Down Expand Up @@ -1254,7 +1287,7 @@ private String getFirstVanityPath(Resource r) {
return vp.length == 0 ? "": vp[0];
}

private Comparator<Resource> vanityResourceComparator = (o1, o2) -> {
private final Comparator<Resource> vanityResourceComparator = (o1, o2) -> {
String s1 = getFirstVanityPath(o1);
String s2 = getFirstVanityPath(o2);
return s1.compareTo(s2);
Expand Down