Skip to content

Commit 85b3435

Browse files
pugnascotiapugnascotia
authored
Fix shadowed vars pt7 (#80996)
Part of #19752. Fix more instances where local variable names were shadowing field names. Also modify our fork of HiddenFieldCheck to add the ignoreConstructorBody and ignoredMethodNames parameters, so that the check can ignore more matches.
1 parent 76e935e commit 85b3435

37 files changed

+227
-153
lines changed

‎build-conventions/src/main/java/org/elasticsearch/gradle/internal/checkstyle/HiddenFieldCheck.java

Lines changed: 60 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -71,9 +71,15 @@ public class HiddenFieldCheck extends AbstractCheck {
7171
/** Control whether to ignore constructor parameters. */
7272
private boolean ignoreConstructorParameter;
7373

74+
/** Control whether to ignore variables in constructor bodies. */
75+
private boolean ignoreConstructorBody;
76+
7477
/** Control whether to ignore parameters of abstract methods. */
7578
private boolean ignoreAbstractMethods;
7679

80+
/** If set, specifies a regex of method names that should be ignored */
81+
private String ignoredMethodNames;
82+
7783
@Override
7884
public int[] getDefaultTokens() {
7985
return getAcceptableTokens();
@@ -224,7 +230,8 @@ private void processVariable(DetailAST ast) {
224230

225231
if ((frame.containsStaticField(name) || isInstanceField(ast, name))
226232
&& isMatchingRegexp(name) == false
227-
&& isIgnoredParam(ast, name) == false) {
233+
&& isIgnoredParam(ast, name) == false
234+
&& isIgnoredVariable(ast, name) == false) {
228235
log(nameAST, MSG_KEY, name);
229236
}
230237
}
@@ -238,7 +245,14 @@ && isIgnoredParam(ast, name) == false) {
238245
* @return true if parameter is ignored.
239246
*/
240247
private boolean isIgnoredParam(DetailAST ast, String name) {
241-
return isIgnoredSetterParam(ast, name) || isIgnoredConstructorParam(ast) || isIgnoredParamOfAbstractMethod(ast);
248+
return isVariableInIgnoredMethod(ast, name)
249+
|| isIgnoredSetterParam(ast, name)
250+
|| isIgnoredConstructorParam(ast)
251+
|| isIgnoredParamOfAbstractMethod(ast);
252+
}
253+
254+
private boolean isIgnoredVariable(DetailAST ast, String name) {
255+
return isIgnoredVariableInConstructorBody(ast, name);
242256
}
243257

244258
/**
@@ -410,6 +424,42 @@ private boolean isIgnoredParamOfAbstractMethod(DetailAST ast) {
410424
return result;
411425
}
412426

427+
/**
428+
* Decides whether to ignore an AST node that is the parameter of a method whose
429+
* name matches the {@link #ignoredMethodNames} regex, if set.
430+
* @param ast the AST to check
431+
* @return true is the ast should be ignored because the parameter belongs to a
432+
* method whose name matches the regex.
433+
*/
434+
private boolean isVariableInIgnoredMethod(DetailAST ast, String name) {
435+
boolean result = false;
436+
if (ignoredMethodNames != null && (ast.getType() == TokenTypes.PARAMETER_DEF || ast.getType() == TokenTypes.VARIABLE_DEF)) {
437+
DetailAST method = ast.getParent();
438+
while (method != null && method.getType() != TokenTypes.METHOD_DEF) {
439+
method = method.getParent();
440+
}
441+
if (method != null && method.getType() == TokenTypes.METHOD_DEF) {
442+
final String methodName = method.findFirstToken(TokenTypes.IDENT).getText();
443+
result = methodName.matches(ignoredMethodNames);
444+
}
445+
}
446+
return result;
447+
}
448+
449+
private boolean isIgnoredVariableInConstructorBody(DetailAST ast, String name) {
450+
boolean result = false;
451+
452+
if (ignoreConstructorBody && ast.getType() == TokenTypes.VARIABLE_DEF) {
453+
DetailAST method = ast.getParent();
454+
while (method != null && method.getType() != TokenTypes.CTOR_DEF) {
455+
method = method.getParent();
456+
}
457+
result = method != null && method.getType() == TokenTypes.CTOR_DEF;
458+
}
459+
460+
return result;
461+
}
462+
413463
/**
414464
* Setter to define the RegExp for names of variables and parameters to ignore.
415465
*
@@ -463,6 +513,14 @@ public void setIgnoreAbstractMethods(boolean ignoreAbstractMethods) {
463513
this.ignoreAbstractMethods = ignoreAbstractMethods;
464514
}
465515

516+
public void setIgnoredMethodNames(String ignoredMethodNames) {
517+
this.ignoredMethodNames = ignoredMethodNames;
518+
}
519+
520+
public void setIgnoreConstructorBody(boolean ignoreConstructorBody) {
521+
this.ignoreConstructorBody = ignoreConstructorBody;
522+
}
523+
466524
/**
467525
* Holds the names of static and instance fields of a type.
468526
*/

‎build-tools-internal/src/main/resources/checkstyle.xml

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -111,10 +111,12 @@
111111
<!--
112112
<module name="org.elasticsearch.gradle.internal.checkstyle.HiddenFieldCheck">
113113
<property name="ignoreConstructorParameter" value="true" />
114+
<property name="ignoreConstructorBody" value="true"/>
114115
<property name="ignoreSetter" value="true" />
115116
<property name="setterCanReturnItsClass" value="true"/>
116-
<property name="ignoreFormat" value="^(threadPool)$"/>
117+
<property name="ignoreFormat" value="^(?:threadPool)$"/>
117118
<property name="ignoreAbstractMethods" value="true"/>
119+
<property name="ignoredMethodNames" value="^(?:createParser)$"/>
118120
<message key="hidden.field" value="''{0}'' hides a field." />
119121
</module>
120122
-->

‎x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/ShardFollowNodeTaskTests.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1421,8 +1421,8 @@ protected void innerSendShardChangesRequest(
14211421
@Override
14221422
protected Scheduler.Cancellable scheduleBackgroundRetentionLeaseRenewal(final LongSupplier followerGlobalCheckpoint) {
14231423
if (scheduleRetentionLeaseRenewal.get()) {
1424-
final ScheduledThreadPoolExecutor scheduler = Scheduler.initScheduler(Settings.EMPTY, "test-scheduler");
1425-
final ScheduledFuture<?> future = scheduler.scheduleWithFixedDelay(
1424+
final ScheduledThreadPoolExecutor testScheduler = Scheduler.initScheduler(Settings.EMPTY, "test-scheduler");
1425+
final ScheduledFuture<?> future = testScheduler.scheduleWithFixedDelay(
14261426
() -> retentionLeaseRenewal.accept(followerGlobalCheckpoint.getAsLong()),
14271427
0,
14281428
TimeValue.timeValueMillis(200).millis(),
@@ -1433,7 +1433,7 @@ protected Scheduler.Cancellable scheduleBackgroundRetentionLeaseRenewal(final Lo
14331433
@Override
14341434
public boolean cancel() {
14351435
final boolean cancel = future.cancel(true);
1436-
scheduler.shutdown();
1436+
testScheduler.shutdown();
14371437
return cancel;
14381438
}
14391439

‎x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/index/engine/FollowingEngineTests.java

Lines changed: 27 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -109,8 +109,8 @@ public void testFollowingEngineRejectsNonFollowingIndex() throws IOException {
109109

110110
public void testIndexSeqNoIsMaintained() throws IOException {
111111
final long seqNo = randomIntBetween(0, Integer.MAX_VALUE);
112-
runIndexTest(seqNo, Engine.Operation.Origin.PRIMARY, (followingEngine, index) -> {
113-
final Engine.IndexResult result = followingEngine.index(index);
112+
runIndexTest(seqNo, Engine.Operation.Origin.PRIMARY, (followingEngine, indexToTest) -> {
113+
final Engine.IndexResult result = followingEngine.index(indexToTest);
114114
assertThat(result.getSeqNo(), equalTo(seqNo));
115115
});
116116
}
@@ -156,8 +156,8 @@ public void runIndexTest(
156156
try (Store store = createStore(shardId, indexSettings, newDirectory())) {
157157
final EngineConfig engineConfig = engineConfig(shardId, indexSettings, threadPool, store);
158158
try (FollowingEngine followingEngine = createEngine(store, engineConfig)) {
159-
final Engine.Index index = indexForFollowing("id", seqNo, origin);
160-
consumer.accept(followingEngine, index);
159+
final Engine.Index indexToTest = indexForFollowing("id", seqNo, origin);
160+
consumer.accept(followingEngine, indexToTest);
161161
}
162162
}
163163
}
@@ -226,16 +226,21 @@ public void testDoNotFillSeqNoGaps() throws Exception {
226226
}
227227

228228
private EngineConfig engineConfig(
229-
final ShardId shardId,
229+
final ShardId shardIdValue,
230230
final IndexSettings indexSettings,
231231
final ThreadPool threadPool,
232232
final Store store
233233
) {
234234
final IndexWriterConfig indexWriterConfig = newIndexWriterConfig();
235235
final Path translogPath = createTempDir("translog");
236-
final TranslogConfig translogConfig = new TranslogConfig(shardId, translogPath, indexSettings, BigArrays.NON_RECYCLING_INSTANCE);
236+
final TranslogConfig translogConfig = new TranslogConfig(
237+
shardIdValue,
238+
translogPath,
239+
indexSettings,
240+
BigArrays.NON_RECYCLING_INSTANCE
241+
);
237242
return new EngineConfig(
238-
shardId,
243+
shardIdValue,
239244
threadPool,
240245
indexSettings,
241246
null,
@@ -331,26 +336,26 @@ private Engine.Delete deleteForPrimary(String id) {
331336
return new Engine.Delete(parsedDoc.id(), EngineTestCase.newUid(parsedDoc), primaryTerm.get());
332337
}
333338

334-
private Engine.Result applyOperation(Engine engine, Engine.Operation op, long primaryTerm, Engine.Operation.Origin origin)
339+
private Engine.Result applyOperation(Engine engine, Engine.Operation op, long primaryTermValue, Engine.Operation.Origin origin)
335340
throws IOException {
336341
final VersionType versionType = origin == Engine.Operation.Origin.PRIMARY ? VersionType.EXTERNAL : null;
337342
final Engine.Result result;
338343
if (op instanceof Engine.Index) {
339-
Engine.Index index = (Engine.Index) op;
344+
Engine.Index engineIndex = (Engine.Index) op;
340345
result = engine.index(
341346
new Engine.Index(
342-
index.uid(),
343-
index.parsedDoc(),
344-
index.seqNo(),
345-
primaryTerm,
346-
index.version(),
347+
engineIndex.uid(),
348+
engineIndex.parsedDoc(),
349+
engineIndex.seqNo(),
350+
primaryTermValue,
351+
engineIndex.version(),
347352
versionType,
348353
origin,
349-
index.startTime(),
350-
index.getAutoGeneratedIdTimestamp(),
351-
index.isRetry(),
352-
index.getIfSeqNo(),
353-
index.getIfPrimaryTerm()
354+
engineIndex.startTime(),
355+
engineIndex.getAutoGeneratedIdTimestamp(),
356+
engineIndex.isRetry(),
357+
engineIndex.getIfSeqNo(),
358+
engineIndex.getIfPrimaryTerm()
354359
)
355360
);
356361
} else if (op instanceof Engine.Delete) {
@@ -360,7 +365,7 @@ private Engine.Result applyOperation(Engine engine, Engine.Operation op, long pr
360365
delete.id(),
361366
delete.uid(),
362367
delete.seqNo(),
363-
primaryTerm,
368+
primaryTermValue,
364369
delete.version(),
365370
versionType,
366371
origin,
@@ -371,7 +376,7 @@ private Engine.Result applyOperation(Engine engine, Engine.Operation op, long pr
371376
);
372377
} else {
373378
Engine.NoOp noOp = (Engine.NoOp) op;
374-
result = engine.noOp(new Engine.NoOp(noOp.seqNo(), primaryTerm, origin, noOp.startTime(), noOp.reason()));
379+
result = engine.noOp(new Engine.NoOp(noOp.seqNo(), primaryTermValue, origin, noOp.startTime(), noOp.reason()));
375380
}
376381
return result;
377382
}
@@ -828,7 +833,7 @@ public void testProcessOnceOnPrimary() throws Exception {
828833
*/
829834
public void testVerifyShardBeforeIndexClosingIsNoOp() throws IOException {
830835
final long seqNo = randomIntBetween(0, Integer.MAX_VALUE);
831-
runIndexTest(seqNo, Engine.Operation.Origin.PRIMARY, (followingEngine, index) -> {
836+
runIndexTest(seqNo, Engine.Operation.Origin.PRIMARY, (followingEngine, indexToTest) -> {
832837
globalCheckpoint.set(randomNonNegativeLong());
833838
try {
834839
followingEngine.verifyEngineBeforeIndexClosing();

‎x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/monitoring/collector/ccr/AutoFollowStatsMonitoringDocTests.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ public void testToXContent() throws IOException {
8888
final NavigableMap<String, AutoFollowedCluster> trackingClusters = new TreeMap<>(
8989
Collections.singletonMap(randomAlphaOfLength(4), new AutoFollowedCluster(1L, 1L))
9090
);
91-
final AutoFollowStats autoFollowStats = new AutoFollowStats(
91+
autoFollowStats = new AutoFollowStats(
9292
randomNonNegativeLong(),
9393
randomNonNegativeLong(),
9494
randomNonNegativeLong(),

‎x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/monitoring/collector/ccr/FollowStatsMonitoringDocTests.java

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,7 @@ public void testToXContent() throws IOException {
117117
)
118118
);
119119
final long timeSinceLastReadMillis = randomNonNegativeLong();
120-
final ShardFollowNodeTaskStatus status = new ShardFollowNodeTaskStatus(
120+
final ShardFollowNodeTaskStatus taskStatus = new ShardFollowNodeTaskStatus(
121121
"leader_cluster",
122122
"leader_index",
123123
"follower_index",
@@ -148,7 +148,7 @@ public void testToXContent() throws IOException {
148148
timeSinceLastReadMillis,
149149
new ElasticsearchException("fatal error")
150150
);
151-
final FollowStatsMonitoringDoc document = new FollowStatsMonitoringDoc("_cluster", timestamp, intervalMillis, node, status);
151+
final FollowStatsMonitoringDoc document = new FollowStatsMonitoringDoc("_cluster", timestamp, intervalMillis, node, taskStatus);
152152
final BytesReference xContent = XContentHelper.toXContent(document, XContentType.JSON, false);
153153
assertThat(
154154
xContent.utf8ToString(),
@@ -273,7 +273,7 @@ public void testShardFollowNodeTaskStatusFieldsMapped() throws IOException {
273273
final NavigableMap<Long, Tuple<Integer, ElasticsearchException>> fetchExceptions = new TreeMap<>(
274274
Collections.singletonMap(1L, Tuple.tuple(2, new ElasticsearchException("shard is sad")))
275275
);
276-
final ShardFollowNodeTaskStatus status = new ShardFollowNodeTaskStatus(
276+
final ShardFollowNodeTaskStatus taskStatus = new ShardFollowNodeTaskStatus(
277277
"remote_cluster",
278278
"leader_index",
279279
"follower_index",
@@ -305,7 +305,7 @@ public void testShardFollowNodeTaskStatusFieldsMapped() throws IOException {
305305
new ElasticsearchException("fatal error")
306306
);
307307
XContentBuilder builder = jsonBuilder();
308-
builder.value(status);
308+
builder.value(taskStatus);
309309
Map<String, Object> serializedStatus = XContentHelper.convertToMap(XContentType.JSON.xContent(), Strings.toString(builder), false);
310310

311311
byte[] loadedTemplate = MonitoringTemplateRegistry.getTemplateConfigForMonitoredSystem(MonitoredSystem.ES).loadBytes();

‎x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/rollup/action/GetRollupIndexCapsAction.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ public String[] indices() {
7474
}
7575

7676
@Override
77-
public IndicesRequest indices(String... indices) {
77+
public IndicesRequest indices(@SuppressWarnings("HiddenField") String... indices) {
7878
Objects.requireNonNull(indices, "indices must not be null");
7979
for (String index : indices) {
8080
Objects.requireNonNull(index, "index must not be null");

‎x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/scheduler/Cron.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -787,7 +787,7 @@ public static void validate(String expression) throws IllegalArgumentException {
787787
//
788788
////////////////////////////////////////////////////////////////////////////
789789

790-
private void buildExpression(String expression) {
790+
private void buildExpression(@SuppressWarnings("HiddenField") String expression) {
791791

792792
try {
793793

‎x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/search/action/AsyncSearchResponse.java

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -100,8 +100,8 @@ public void writeTo(StreamOutput out) throws IOException {
100100
out.writeLong(expirationTimeMillis);
101101
}
102102

103-
public AsyncSearchResponse clone(String id) {
104-
return new AsyncSearchResponse(id, searchResponse, error, isPartial, false, startTimeMillis, expirationTimeMillis);
103+
public AsyncSearchResponse clone(String searchId) {
104+
return new AsyncSearchResponse(searchId, searchResponse, error, isPartial, false, startTimeMillis, expirationTimeMillis);
105105
}
106106

107107
/**
@@ -165,8 +165,8 @@ public long getExpirationTime() {
165165
}
166166

167167
@Override
168-
public AsyncSearchResponse withExpirationTime(long expirationTimeMillis) {
169-
return new AsyncSearchResponse(id, searchResponse, error, isPartial, isRunning, startTimeMillis, expirationTimeMillis);
168+
public AsyncSearchResponse withExpirationTime(long expirationTime) {
169+
return new AsyncSearchResponse(id, searchResponse, error, isPartial, isRunning, startTimeMillis, expirationTime);
170170
}
171171

172172
@Override

‎x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/CommandLineHttpClient.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -343,7 +343,7 @@ public static String apiKeyHeaderValue(SecureString apiKey) {
343343
* Returns a TrustManager to be used in a client SSLContext, which trusts all certificates that are signed
344344
* by a specific CA certificate ( identified by its SHA256 fingerprint, {@code pinnedCaCertFingerPrint} )
345345
*/
346-
private TrustManager fingerprintTrustingTrustManager(String pinnedCaCertFingerprint) {
346+
private TrustManager fingerprintTrustingTrustManager(String caCertFingerprint) {
347347
final TrustManager trustManager = new X509TrustManager() {
348348
public void checkClientTrusted(X509Certificate[] chain, String authType) throws CertificateException {}
349349

@@ -354,7 +354,7 @@ public void checkServerTrusted(X509Certificate[] chain, String authType) throws
354354
final Certificate caCertFromChain = chain[1];
355355
MessageDigest sha256 = MessageDigests.sha256();
356356
sha256.update(caCertFromChain.getEncoded());
357-
if (MessageDigests.toHexString(sha256.digest()).equals(pinnedCaCertFingerprint) == false) {
357+
if (MessageDigests.toHexString(sha256.digest()).equals(caCertFingerprint) == false) {
358358
throw new CertificateException();
359359
}
360360
}

‎x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/InvalidateApiKeyRequest.java

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -249,14 +249,16 @@ public int hashCode() {
249249
return Objects.hash(realmName, userName, ids, name, ownedByAuthenticatedUser);
250250
}
251251

252-
private void validateIds(@Nullable String[] ids) {
253-
if (ids != null) {
254-
if (ids.length == 0) {
252+
private void validateIds(@Nullable String[] idsToValidate) {
253+
if (idsToValidate != null) {
254+
if (idsToValidate.length == 0) {
255255
final ActionRequestValidationException validationException = new ActionRequestValidationException();
256256
validationException.addValidationError("Field [ids] cannot be an empty array");
257257
throw validationException;
258258
} else {
259-
final int[] idxOfBlankIds = IntStream.range(0, ids.length).filter(i -> Strings.hasText(ids[i]) == false).toArray();
259+
final int[] idxOfBlankIds = IntStream.range(0, idsToValidate.length)
260+
.filter(i -> Strings.hasText(idsToValidate[i]) == false)
261+
.toArray();
260262
if (idxOfBlankIds.length > 0) {
261263
final ActionRequestValidationException validationException = new ActionRequestValidationException();
262264
validationException.addValidationError(

‎x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/role/PutRoleRequest.java

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -95,9 +95,9 @@ public ActionRequestValidationException validate() {
9595
} catch (IllegalArgumentException e) {
9696
validationException = addValidationError(e.getMessage(), validationException);
9797
}
98-
for (String name : privilege.getPrivileges()) {
98+
for (String privilegeName : privilege.getPrivileges()) {
9999
try {
100-
ApplicationPrivilege.validatePrivilegeOrActionName(name);
100+
ApplicationPrivilege.validatePrivilegeOrActionName(privilegeName);
101101
} catch (IllegalArgumentException e) {
102102
validationException = addValidationError(e.getMessage(), validationException);
103103
}
@@ -117,12 +117,12 @@ public void name(String name) {
117117
this.name = name;
118118
}
119119

120-
public void cluster(String... clusterPrivileges) {
121-
this.clusterPrivileges = clusterPrivileges;
120+
public void cluster(String... clusterPrivilegesArray) {
121+
this.clusterPrivileges = clusterPrivilegesArray;
122122
}
123123

124-
public void conditionalCluster(ConfigurableClusterPrivilege... configurableClusterPrivileges) {
125-
this.configurableClusterPrivileges = configurableClusterPrivileges;
124+
public void conditionalCluster(ConfigurableClusterPrivilege... configurableClusterPrivilegesArray) {
125+
this.configurableClusterPrivileges = configurableClusterPrivilegesArray;
126126
}
127127

128128
public void addIndex(RoleDescriptor.IndicesPrivileges... privileges) {

0 commit comments

Comments
 (0)