Skip to content

Commit 4d2bb1f

Browse files
mferretticlaude
andcommitted
Address Copilot review: null guards, fix dead code, add RECIPE_MAP growth note
- SafeFetchResolver.resolve(): guard against null root - resolveFromMethodOn(): guard null root before NamedProviderCoercedResolver branch - resolveExpression inner: guard null root before dotIndex provider lookup - RECIPE_MAP: expand Javadoc noting bounded-in-practice growth - accessor(): fix dead-code `methods == null` check → `methods.isEmpty()` - accessor(): fix two "Didn't accessor" log typos → "Didn't find accessor" - SharedFakeValuesServiceTest: call shutdownNow() on awaitTermination timeout Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent d41e230 commit 4d2bb1f

2 files changed

Lines changed: 20 additions & 8 deletions

File tree

src/main/java/net/datafaker/service/FakeValuesService.java

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,12 @@ public class FakeValuesService {
8181

8282
private static final Map<String, String[]> EXPRESSION_2_SPLITTED = new CopyOnWriteMap<>(WeakHashMap::new);
8383

84-
/** L1: static recipe cache — context-free resolvers shared across all Fakers with same locale. */
84+
/**
85+
* L1: static recipe cache — context-free resolvers shared across all Fakers with same locale.
86+
* Growth is bounded in practice by unique YAML expressions × locales. User-supplied dynamic
87+
* expressions via {@code faker.expression()} carry the same theoretical unbounded-growth exposure
88+
* as the other static string caches in this class (NAME_2_YAML, EXPRESSION_2_SPLITTED, etc.).
89+
*/
8590
private static final Map<CacheKey, ValueResolver> RECIPE_MAP = new CopyOnWriteMap<>(HashMap::new);
8691

8792
/** L2: per-instance materialized cache — resolvers pre-bound to this Faker's providers for fast repeated calls. */
@@ -179,6 +184,7 @@ private SafeFetchResolver(String simpleDirective) {
179184

180185
@Override
181186
public Object resolve(ProviderRegistration root, FakerContext context) {
187+
if (root == null) return null;
182188
return root.fakeValuesService().safeFetch(simpleDirective, context, null);
183189
}
184190

@@ -759,7 +765,7 @@ private Object resolveExpression(String directive, String[] args, Object current
759765
}
760766
res.add(resolveFromMethodOn(current, directive, args, root));
761767
}
762-
if (dotIndex > 0) {
768+
if (dotIndex > 0 && root != null) {
763769
String providerClassName = directive.substring(0, dotIndex);
764770
String methodName = directive.substring(dotIndex + 1);
765771
AbstractProvider<?> ap = root.getProvider(providerClassName);
@@ -889,7 +895,7 @@ private ValueResolver resolveFromMethodOn(Object obj, String directive, String[]
889895
if (obj instanceof ProviderRegistration) {
890896
return new RootCoercedResolver(accessor);
891897
}
892-
if (obj instanceof AbstractProvider) {
898+
if (obj instanceof AbstractProvider && root != null) {
893899
String providerName = obj.getClass().getSimpleName();
894900
Object registered = root.getProvider(providerName);
895901
if (registered != null && registered.getClass() == obj.getClass()) {
@@ -967,8 +973,8 @@ private MethodAndCoercedArgs accessor(Class<?> clazz, final String accessorName,
967973
});
968974

969975
Collection<Method> methods = classMethodsMap.getOrDefault(name, emptyList());
970-
if (methods == null) {
971-
LOG.fine(() -> "Didn't accessor named %s on %s with args %s (methods=%s)".formatted(accessorName, clazz.getSimpleName(), Arrays.toString(args), null));
976+
if (methods.isEmpty()) {
977+
LOG.fine(() -> "Didn't find accessor named %s on %s with args %s".formatted(accessorName, clazz.getSimpleName(), Arrays.toString(args)));
972978
return null;
973979
}
974980
LOG.fine(() -> "Found accessor named %s on %s in cache: %s".formatted(accessorName, clazz.getSimpleName(), methods));
@@ -987,7 +993,7 @@ private MethodAndCoercedArgs accessor(Class<?> clazz, final String accessorName,
987993
if (mostRestrictive != null) {
988994
return new MethodAndCoercedArgs(mostRestrictive, coercedArgumentsForMostRestrictive);
989995
}
990-
LOG.fine(() -> "Didn't accessor named %s on %s with args %s (methods=%s)".formatted(accessorName, clazz.getSimpleName(), Arrays.toString(args), methods));
996+
LOG.fine(() -> "Didn't find accessor named %s on %s with args %s (methods=%s)".formatted(accessorName, clazz.getSimpleName(), Arrays.toString(args), methods));
991997
return null;
992998
}
993999

src/test/java/net/datafaker/SharedFakeValuesServiceTest.java

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,10 @@ void concurrentFakersProduceNoErrors() throws Exception {
3737
}));
3838
}
3939
pool.shutdown();
40-
assertThat(pool.awaitTermination(120, TimeUnit.SECONDS)).isTrue();
40+
if (!pool.awaitTermination(120, TimeUnit.SECONDS)) {
41+
pool.shutdownNow();
42+
throw new AssertionError("Thread pool did not terminate within timeout");
43+
}
4144
for (Future<Void> f : futures) {
4245
f.get();
4346
}
@@ -63,7 +66,10 @@ void multipleLocalesConcurrentlyProduceNoErrors() throws Exception {
6366
}));
6467
}
6568
pool.shutdown();
66-
assertThat(pool.awaitTermination(60, TimeUnit.SECONDS)).isTrue();
69+
if (!pool.awaitTermination(60, TimeUnit.SECONDS)) {
70+
pool.shutdownNow();
71+
throw new AssertionError("Thread pool did not terminate within timeout");
72+
}
6773
for (Future<Void> f : futures) {
6874
f.get();
6975
}

0 commit comments

Comments
 (0)