Skip to content

Commit 5a171b8

Browse files
jordanpadamsclaude
jordanpadams
and
claude
committed
Address code review: test real DataLoader via reflection, apply Google style
- Replace DataLoaderTestHelper (which duplicated production logic) with a StubConnectionFactory + reflection-based invokeProcessErrors() that calls the real DataLoader.processErrors(). Tests now fail if the production conflict-handling is changed or setIgnoreConflicts wiring is removed. - Fix empty logErrors() body in anonymous Response.Bulk stub (SonarCube S1186). - Reformat TestDataLoaderIgnoreConflicts.java to Google Java Style Guide (2-space indent, 100-col limit, brace style, import ordering). - Document Google Java Style Guide requirement in CLAUDE.md. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent f61b7d6 commit 5a171b8

2 files changed

Lines changed: 194 additions & 118 deletions

File tree

‎CLAUDE.md‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,16 @@ The library provides a unified `Request` and `Response` interface that abstracts
114114
- Builds publish to Maven Central via Sonatype Central Portal
115115
- Requires secrets: `CENTRAL_REPOSITORY_USERNAME`, `CENTRAL_REPOSITORY_TOKEN`, `CODE_SIGNING_KEY`
116116

117+
## Coding Style
118+
119+
All Java source files must follow the [Google Java Style Guide](https://google.github.io/styleguide/javaguide.html). Key rules:
120+
121+
- **Indentation**: 2 spaces (no tabs)
122+
- **Column limit**: 100 characters
123+
- **Braces**: always used for blocks, opening brace on same line
124+
- **Imports**: no wildcard imports; ordered as static imports first, then standard library, then third-party, then project imports, each group separated by a blank line
125+
- **Naming**: `UpperCamelCase` for classes, `lowerCamelCase` for methods and variables, `UPPER_SNAKE_CASE` for constants
126+
117127
## Critical Invariants
118128

119129
**Schema field must exist before metadata is loaded:**

‎src/test/java/dao/TestDataLoaderIgnoreConflicts.java‎

Lines changed: 184 additions & 118 deletions
Original file line numberDiff line numberDiff line change
@@ -1,157 +1,223 @@
11
package dao;
22

3+
import gov.nasa.pds.registry.common.ConnectionFactory;
34
import gov.nasa.pds.registry.common.Response;
5+
import gov.nasa.pds.registry.common.RestClient;
46
import gov.nasa.pds.registry.common.es.dao.DataLoader;
57
import gov.nasa.pds.registry.common.es.dao.dd.LddVersions;
6-
import org.junit.jupiter.api.Test;
7-
8+
import java.io.IOException;
9+
import java.lang.reflect.Method;
810
import java.time.Instant;
911
import java.util.Arrays;
10-
import java.util.HashSet;
1112
import java.util.LinkedHashMap;
1213
import java.util.List;
1314
import java.util.Set;
15+
import org.apache.http.HttpHost;
16+
import org.apache.http.client.CredentialsProvider;
17+
import org.junit.jupiter.api.Test;
1418

15-
import static org.junit.jupiter.api.Assertions.*;
19+
import static org.junit.jupiter.api.Assertions.assertEquals;
20+
import static org.junit.jupiter.api.Assertions.assertNotNull;
21+
import static org.junit.jupiter.api.Assertions.assertTrue;
1622

1723
/**
1824
* Tests for DataLoader.ignoreConflicts flag and related behaviour.
1925
*
20-
* These tests exercise processErrors() indirectly through the package-visible
21-
* processErrors overload exposed below, without needing a live Elasticsearch cluster.
26+
* <p>These tests call the real DataLoader.processErrors() via reflection so that production
27+
* conflict-handling logic is actually exercised, not duplicated.
2228
*/
2329
public class TestDataLoaderIgnoreConflicts {
2430

25-
// Minimal Response.Bulk.Item stub
26-
private static Response.Bulk.Item item(String operation, int status, boolean error, String id) {
27-
return new Response.Bulk.Item() {
28-
public boolean error() { return error; }
29-
public String id() { return id; }
30-
public String index() { return "test"; }
31-
public String operation() { return operation; }
32-
public String reason() { return "conflict"; }
33-
public String result() { return null; }
34-
public int status() { return status; }
35-
};
31+
// Minimal Response.Bulk.Item stub
32+
private static Response.Bulk.Item item(
33+
String operation, int status, boolean error, String id) {
34+
return new Response.Bulk.Item() {
35+
public boolean error() {
36+
return error;
37+
}
38+
39+
public String id() {
40+
return id;
41+
}
42+
43+
public String index() {
44+
return "test";
45+
}
46+
47+
public String operation() {
48+
return operation;
49+
}
50+
51+
public String reason() {
52+
return "conflict";
53+
}
54+
55+
public String result() {
56+
return null;
57+
}
58+
59+
public int status() {
60+
return status;
61+
}
62+
};
63+
}
64+
65+
// Minimal Response.Bulk stub
66+
private static Response.Bulk bulkResponse(boolean errors, List<Response.Bulk.Item> items) {
67+
return new Response.Bulk() {
68+
public boolean errors() {
69+
return errors;
70+
}
71+
72+
public List<Response.Bulk.Item> items() {
73+
return items;
74+
}
75+
76+
public void logErrors() { /* no-op stub */ }
77+
78+
public long took() {
79+
return 0;
80+
}
81+
};
82+
}
83+
84+
/**
85+
* Constructs a real DataLoader with a no-op ConnectionFactory stub and sets ignoreConflicts via
86+
* the public setter. Exposes processErrors() via reflection.
87+
*/
88+
private static int invokeProcessErrors(
89+
boolean ignoreConflicts,
90+
Response.Bulk resp,
91+
Set<String> errorLidvids,
92+
LinkedHashMap<String, String> todo,
93+
int retry)
94+
throws Exception {
95+
ConnectionFactory stub = new StubConnectionFactory();
96+
DataLoader loader = new DataLoader(stub);
97+
loader.setIgnoreConflicts(ignoreConflicts);
98+
99+
Method m =
100+
DataLoader.class.getDeclaredMethod(
101+
"processErrors", Response.Bulk.class, Set.class, LinkedHashMap.class, int.class);
102+
m.setAccessible(true);
103+
return (int) m.invoke(loader, resp, errorLidvids, todo, retry);
104+
}
105+
106+
@Test
107+
void lddVersions_defaultLastDate_isPublicConstant() {
108+
assertNotNull(LddVersions.DEFAULT_LAST_DATE);
109+
assertEquals(
110+
LddVersions.DEFAULT_LAST_DATE,
111+
new LddVersions().lastDate,
112+
"DEFAULT_LAST_DATE must equal the initial lastDate of a new LddVersions");
113+
assertEquals(
114+
Instant.parse(LddVersions.DEFAULT_DATE),
115+
LddVersions.DEFAULT_LAST_DATE,
116+
"DEFAULT_LAST_DATE and DEFAULT_DATE must represent the same instant");
117+
}
118+
119+
@Test
120+
void processErrors_409_ignoreConflicts_false_countsAsError() throws Exception {
121+
Response.Bulk.Item conflict409 = item("create", 409, true, "urn:test::1.0");
122+
Response.Bulk resp = bulkResponse(true, Arrays.asList(conflict409));
123+
LinkedHashMap<String, String> todo = new LinkedHashMap<>();
124+
todo.put("{\"create\":{\"_id\":\"urn:test::1.0\"}}", "{}");
125+
126+
int errors = invokeProcessErrors(false, resp, null, todo, 0);
127+
128+
assertEquals(1, errors, "409 with ignoreConflicts=false must count as 1 error");
129+
assertTrue(todo.isEmpty(), "409 item must be removed from todo regardless of ignoreConflicts");
130+
}
131+
132+
@Test
133+
void processErrors_409_ignoreConflicts_true_notCountedAsError() throws Exception {
134+
Response.Bulk.Item conflict409 = item("create", 409, true, "urn:test::1.0");
135+
Response.Bulk resp = bulkResponse(true, Arrays.asList(conflict409));
136+
LinkedHashMap<String, String> todo = new LinkedHashMap<>();
137+
todo.put("{\"create\":{\"_id\":\"urn:test::1.0\"}}", "{}");
138+
139+
int errors = invokeProcessErrors(true, resp, null, todo, 0);
140+
141+
assertEquals(0, errors, "409 with ignoreConflicts=true must not count as an error");
142+
assertTrue(todo.isEmpty(), "409 item must still be removed from todo");
143+
}
144+
145+
@Test
146+
void processErrors_nonConflictError_alwaysCountsRegardlessOfFlag() throws Exception {
147+
for (boolean flag : new boolean[] {false, true}) {
148+
Response.Bulk.Item serverError = item("index", 500, true, "urn:test::1.0");
149+
Response.Bulk resp = bulkResponse(true, Arrays.asList(serverError));
150+
LinkedHashMap<String, String> todo = new LinkedHashMap<>();
151+
todo.put("{\"index\":{\"_id\":\"urn:test::1.0\"}}", "{}");
152+
153+
int errors = invokeProcessErrors(flag, resp, null, todo, 0);
154+
155+
assertEquals(
156+
1,
157+
errors,
158+
"Non-409 error must always count regardless of ignoreConflicts=" + flag);
36159
}
160+
}
37161

38-
// Minimal Response.Bulk stub
39-
private static Response.Bulk bulkResponse(boolean errors, List<Response.Bulk.Item> items) {
40-
return new Response.Bulk() {
41-
public boolean errors() { return errors; }
42-
public List<Response.Bulk.Item> items() { return items; }
43-
public void logErrors() {}
44-
public long took() { return 0; }
45-
};
46-
}
162+
@Test
163+
void processErrors_successItem_zeroErrors() throws Exception {
164+
Response.Bulk.Item success = item("index", 200, false, "urn:test::1.0");
165+
Response.Bulk resp = bulkResponse(false, Arrays.asList(success));
166+
LinkedHashMap<String, String> todo = new LinkedHashMap<>();
167+
todo.put("{\"index\":{\"_id\":\"urn:test::1.0\"}}", "{}");
47168

48-
@Test
49-
void lddVersions_defaultLastDate_isPublicConstant() {
50-
// Confirms the sentinel is accessible and matches the Instant stored in new LddVersions()
51-
assertNotNull(LddVersions.DEFAULT_LAST_DATE);
52-
assertEquals(LddVersions.DEFAULT_LAST_DATE, new LddVersions().lastDate,
53-
"DEFAULT_LAST_DATE must equal the initial lastDate of a new LddVersions");
54-
assertEquals(Instant.parse(LddVersions.DEFAULT_DATE), LddVersions.DEFAULT_LAST_DATE,
55-
"DEFAULT_LAST_DATE and DEFAULT_DATE must represent the same instant");
56-
}
169+
int errors = invokeProcessErrors(false, resp, null, todo, 0);
57170

58-
@Test
59-
void processErrors_409_ignoreConflicts_false_countsAsError() throws Exception {
60-
// Default behaviour: 409 on create is counted as an error (product ingestion)
61-
DataLoaderTestHelper helper = new DataLoaderTestHelper(false);
62-
Response.Bulk.Item conflict409 = item("create", 409, true, "urn:test::1.0");
63-
Response.Bulk resp = bulkResponse(true, Arrays.asList(conflict409));
64-
LinkedHashMap<String, String> todo = new LinkedHashMap<>();
65-
todo.put("{\"create\":{\"_id\":\"urn:test::1.0\"}}", "{}");
171+
assertEquals(0, errors);
172+
assertTrue(todo.isEmpty());
173+
}
66174

67-
int errors = helper.processErrors(resp, null, todo, 0);
175+
/**
176+
* Minimal ConnectionFactory stub. Only getIndexName() is called by processErrors() (via the
177+
* debug log path when ignoreConflicts=true). All other methods are unused in these unit tests.
178+
*/
179+
private static class StubConnectionFactory implements ConnectionFactory {
68180

69-
assertEquals(1, errors, "409 with ignoreConflicts=false must count as 1 error");
70-
assertTrue(todo.isEmpty(), "409 item must be removed from todo regardless of ignoreConflicts");
181+
public ConnectionFactory clone() {
182+
return this;
71183
}
72184

73-
@Test
74-
void processErrors_409_ignoreConflicts_true_notCountedAsError() throws Exception {
75-
// LDD load behaviour: 409 on create is NOT an error — document already exists
76-
DataLoaderTestHelper helper = new DataLoaderTestHelper(true);
77-
Response.Bulk.Item conflict409 = item("create", 409, true, "urn:test::1.0");
78-
Response.Bulk resp = bulkResponse(true, Arrays.asList(conflict409));
79-
LinkedHashMap<String, String> todo = new LinkedHashMap<>();
80-
todo.put("{\"create\":{\"_id\":\"urn:test::1.0\"}}", "{}");
81-
82-
int errors = helper.processErrors(resp, null, todo, 0);
185+
public RestClient createRestClient() {
186+
throw new UnsupportedOperationException();
187+
}
83188

84-
assertEquals(0, errors, "409 with ignoreConflicts=true must not count as an error");
85-
assertTrue(todo.isEmpty(), "409 item must still be removed from todo");
189+
public CredentialsProvider getCredentials() {
190+
return null;
86191
}
87192

88-
@Test
89-
void processErrors_nonConflictError_alwaysCountsRegardlessOfFlag() throws Exception {
90-
// A non-409 error must always count, regardless of ignoreConflicts
91-
for (boolean flag : new boolean[]{false, true}) {
92-
DataLoaderTestHelper helper = new DataLoaderTestHelper(flag);
93-
Response.Bulk.Item serverError = item("index", 500, true, "urn:test::1.0");
94-
Response.Bulk resp = bulkResponse(true, Arrays.asList(serverError));
95-
LinkedHashMap<String, String> todo = new LinkedHashMap<>();
96-
todo.put("{\"index\":{\"_id\":\"urn:test::1.0\"}}", "{}");
193+
public org.apache.hc.client5.http.auth.CredentialsProvider getCredentials5() {
194+
return null;
195+
}
97196

98-
int errors = helper.processErrors(resp, null, todo, 0);
197+
public HttpHost getHost() {
198+
return null;
199+
}
99200

100-
assertEquals(1, errors, "Non-409 error must always count regardless of ignoreConflicts=" + flag);
101-
}
201+
public org.apache.hc.core5.http.HttpHost getHost5() {
202+
return null;
102203
}
103204

104-
@Test
105-
void processErrors_successItem_zeroErrors() throws Exception {
106-
DataLoaderTestHelper helper = new DataLoaderTestHelper(false);
107-
Response.Bulk.Item success = item("index", 200, false, "urn:test::1.0");
108-
Response.Bulk resp = bulkResponse(false, Arrays.asList(success));
109-
LinkedHashMap<String, String> todo = new LinkedHashMap<>();
110-
todo.put("{\"index\":{\"_id\":\"urn:test::1.0\"}}", "{}");
205+
public String getHostName() {
206+
return "stub-host";
207+
}
111208

112-
int errors = helper.processErrors(resp, null, todo, 0);
209+
public String getIndexName() {
210+
return "stub-index";
211+
}
113212

114-
assertEquals(0, errors);
115-
assertTrue(todo.isEmpty());
213+
public boolean isTrustingSelfSigned() {
214+
return false;
116215
}
117216

118-
/**
119-
* Test-only subclass that exposes processErrors for unit testing without a live cluster.
120-
* DataLoader is in a different package so we use reflection to set the ignoreConflicts field.
121-
*/
122-
static class DataLoaderTestHelper {
123-
private final boolean ignoreConflicts;
124-
125-
DataLoaderTestHelper(boolean ignoreConflicts) {
126-
this.ignoreConflicts = ignoreConflicts;
127-
}
128-
129-
int processErrors(Response.Bulk resp, Set<String> errorLidvids,
130-
LinkedHashMap<String, String> todo, int retry) {
131-
int numErrors = 0;
132-
if (resp.errors()) {
133-
for (Response.Bulk.Item item : resp.items()) {
134-
if (item.error()) {
135-
if (item.operation().equals("create") && item.status() == 409) {
136-
todo.remove("{\"create\":{\"_id\":\"" + item.id() + "\"}}");
137-
if (ignoreConflicts) {
138-
// treated as success — not counted
139-
} else {
140-
numErrors++;
141-
}
142-
} else {
143-
numErrors++;
144-
todo.remove("{\"index\":{\"_id\":\"" + item.id() + "\"}}");
145-
if (errorLidvids != null) errorLidvids.add(item.id());
146-
}
147-
} else {
148-
todo.remove("{\"index\":{\"_id\":\"" + item.id() + "\"}}");
149-
}
150-
}
151-
} else {
152-
todo.clear();
153-
}
154-
return numErrors;
155-
}
217+
public void reconnect() throws IOException, InterruptedException { /* no-op */ }
218+
219+
public ConnectionFactory setIndexName(String idxName) {
220+
return this;
156221
}
222+
}
157223
}

0 commit comments

Comments
 (0)