Skip to content

Commit

Permalink
Checkstyle shadows vars pt8 (#81147) (#81250)
Browse files Browse the repository at this point in the history 
Part of #19752. Fix more instances where local variable names were
shadowing field names. Also expand the possible method names that are
skipped when checking for shadowed vars, and allow shadowed vars in
builder classes.
  • Loading branch information
@pugnascotia
pugnascotia authored Dec 2, 2021
1 parent c23f8f6 commit 6fc8166
Show file tree
Hide file tree
Showing 58 changed files with 254 additions and 224 deletions.
42 changes: 26 additions & 16 deletions ...ventions/src/main/java/org/elasticsearch/gradle/internal/checkstyle/HiddenFieldCheck.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import com.puppycrawl.tools.checkstyle.utils.TokenUtil;

import java.util.HashSet;
import java.util.List;
import java.util.Locale;
import java.util.Objects;
import java.util.Set;
Expand Down Expand Up @@ -342,24 +343,33 @@ private boolean isSetterMethod(DetailAST aMethodAST, String aName) {
boolean isSetterMethod = false;

// ES also allows setters with the same name as a property, and builder-style settings that start with "with".
if (("set" + capitalize(aName)).equals(methodName) || ("with" + capitalize(aName)).equals(methodName) || aName.equals(methodName)) {
final List<String> possibleSetterNames = List.of(
"set" + capitalize(aName, true),
"set" + capitalize(aName, false),
"with" + capitalize(aName, true),
"with" + capitalize(aName, false),
aName
);

if (possibleSetterNames.contains(methodName)) {
// method name did match set${Name}(${anyType} ${aName})
// where ${Name} is capitalized version of ${aName}
// therefore this method is potentially a setter
final DetailAST typeAST = aMethodAST.findFirstToken(TokenTypes.TYPE);
final String returnType = typeAST.getFirstChild().getText();
if (typeAST.findFirstToken(TokenTypes.LITERAL_VOID) != null || setterCanReturnItsClass && frame.isEmbeddedIn(returnType)) {
// this method has signature
//
// void set${Name}(${anyType} ${name})
//
// and therefore considered to be a setter
//
// or
//
// return type is not void, but it is the same as the class
// where method is declared and and mSetterCanReturnItsClass
// is set to true

// The method is named `setFoo`, `withFoo`, or just `foo` and returns void
final boolean returnsVoid = typeAST.findFirstToken(TokenTypes.LITERAL_VOID) != null;

// Or the method is named as above, and returns the class type or a builder type.
// It ought to be possible to see if we're in a `${returnType}.Builder`, but for some reason the parse
// tree has `returnType` as `.` when the current class is `Builder` so instead assume that a class called `Builder` is OK.
final boolean returnsSelf = setterCanReturnItsClass && frame.isEmbeddedIn(returnType);

final boolean returnsBuilder = setterCanReturnItsClass
&& (frame.isEmbeddedIn(returnType + "Builder") || (frame.isEmbeddedIn("Builder")));

if (returnsVoid || returnsSelf || returnsBuilder) {
isSetterMethod = true;
}
}
Expand All @@ -374,13 +384,13 @@ private boolean isSetterMethod(DetailAST aMethodAST, String aName) {
* @param name a property name
* @return capitalized property name
*/
private static String capitalize(final String name) {
private static String capitalize(final String name, boolean javaBeanCompliant) {
String setterName = name;
// we should not capitalize the first character if the second
// one is a capital one, since according to JavaBeans spec
// setXYzz() is a setter for XYzz property, not for xYzz one.
// @pugnascotia: unless the first char is 'x'.
if (name.length() == 1 || (Character.isUpperCase(name.charAt(1)) == false || name.charAt(0) == 'x')) {
// @pugnascotia: this is unhelpful in the Elasticsearch codebase. We have e.g. xContent -> setXContent, or nNeighbors -> nNeighbors.
if (name.length() == 1 || (javaBeanCompliant == false || Character.isUpperCase(name.charAt(1)) == false)) {
setterName = name.substring(0, 1).toUpperCase(Locale.ENGLISH) + name.substring(1);
}
return setterName;
Expand Down
46 changes: 23 additions & 23 deletions server/src/internalClusterTest/java/org/elasticsearch/action/IndicesRequestIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -282,31 +282,31 @@ public void testBulk() {
String[] bulkShardActions = new String[] { BulkAction.NAME + "[s][p]", BulkAction.NAME + "[s][r]" };
interceptTransportActions(bulkShardActions);

List<String> indices = new ArrayList<>();
List<String> indicesOrAliases = new ArrayList<>();
BulkRequest bulkRequest = new BulkRequest();
int numIndexRequests = iterations(1, 10);
for (int i = 0; i < numIndexRequests; i++) {
String indexOrAlias = randomIndexOrAlias();
bulkRequest.add(new IndexRequest(indexOrAlias, "type", "id").source(Requests.INDEX_CONTENT_TYPE, "field", "value"));
indices.add(indexOrAlias);
indicesOrAliases.add(indexOrAlias);
}
int numDeleteRequests = iterations(1, 10);
for (int i = 0; i < numDeleteRequests; i++) {
String indexOrAlias = randomIndexOrAlias();
bulkRequest.add(new DeleteRequest(indexOrAlias, "type", "id"));
indices.add(indexOrAlias);
indicesOrAliases.add(indexOrAlias);
}
int numUpdateRequests = iterations(1, 10);
for (int i = 0; i < numUpdateRequests; i++) {
String indexOrAlias = randomIndexOrAlias();
bulkRequest.add(new UpdateRequest(indexOrAlias, "type", "id").doc(Requests.INDEX_CONTENT_TYPE, "field1", "value1"));
indices.add(indexOrAlias);
indicesOrAliases.add(indexOrAlias);
}

internalCluster().coordOnlyNodeClient().bulk(bulkRequest).actionGet();

clearInterceptedActions();
assertIndicesSubset(indices, bulkShardActions);
assertIndicesSubset(indicesOrAliases, bulkShardActions);
}

public void testGet() {
Expand Down Expand Up @@ -346,36 +346,36 @@ public void testMultiTermVector() {
String multiTermVectorsShardAction = MultiTermVectorsAction.NAME + "[shard][s]";
interceptTransportActions(multiTermVectorsShardAction);

List<String> indices = new ArrayList<>();
List<String> indicesOrAliases = new ArrayList<>();
MultiTermVectorsRequest multiTermVectorsRequest = new MultiTermVectorsRequest();
int numDocs = iterations(1, 30);
for (int i = 0; i < numDocs; i++) {
String indexOrAlias = randomIndexOrAlias();
multiTermVectorsRequest.add(indexOrAlias, "type", Integer.toString(i));
indices.add(indexOrAlias);
indicesOrAliases.add(indexOrAlias);
}
internalCluster().coordOnlyNodeClient().multiTermVectors(multiTermVectorsRequest).actionGet();

clearInterceptedActions();
assertIndicesSubset(indices, multiTermVectorsShardAction);
assertIndicesSubset(indicesOrAliases, multiTermVectorsShardAction);
}

public void testMultiGet() {
String multiGetShardAction = MultiGetAction.NAME + "[shard][s]";
interceptTransportActions(multiGetShardAction);

List<String> indices = new ArrayList<>();
List<String> indicesOrAliases = new ArrayList<>();
MultiGetRequest multiGetRequest = new MultiGetRequest();
int numDocs = iterations(1, 30);
for (int i = 0; i < numDocs; i++) {
String indexOrAlias = randomIndexOrAlias();
multiGetRequest.add(indexOrAlias, "type", Integer.toString(i));
indices.add(indexOrAlias);
indicesOrAliases.add(indexOrAlias);
}
internalCluster().coordOnlyNodeClient().multiGet(multiGetRequest).actionGet();

clearInterceptedActions();
assertIndicesSubset(indices, multiGetShardAction);
assertIndicesSubset(indicesOrAliases, multiGetShardAction);
}

public void testFlush() {
Expand All @@ -389,9 +389,9 @@ public void testFlush() {
internalCluster().coordOnlyNodeClient().admin().indices().flush(flushRequest).actionGet();

clearInterceptedActions();
String[] indices = TestIndexNameExpressionResolver.newInstance()
String[] concreteIndexNames = TestIndexNameExpressionResolver.newInstance()
.concreteIndexNames(client().admin().cluster().prepareState().get().getState(), flushRequest);
assertIndicesSubset(Arrays.asList(indices), indexShardActions);
assertIndicesSubset(Arrays.asList(concreteIndexNames), indexShardActions);
}

public void testForceMerge() {
Expand All @@ -416,9 +416,9 @@ public void testRefresh() {
internalCluster().coordOnlyNodeClient().admin().indices().refresh(refreshRequest).actionGet();

clearInterceptedActions();
String[] indices = TestIndexNameExpressionResolver.newInstance()
String[] concreteIndexNames = TestIndexNameExpressionResolver.newInstance()
.concreteIndexNames(client().admin().cluster().prepareState().get().getState(), refreshRequest);
assertIndicesSubset(Arrays.asList(indices), indexShardActions);
assertIndicesSubset(Arrays.asList(concreteIndexNames), indexShardActions);
}

public void testClearCache() {
Expand Down Expand Up @@ -675,21 +675,21 @@ private String randomIndexOrAlias() {

private String[] randomIndicesOrAliases() {
int count = randomIntBetween(1, indices.size() * 2); // every index has an alias
String[] indices = new String[count];
String[] randomNames = new String[count];
for (int i = 0; i < count; i++) {
indices[i] = randomIndexOrAlias();
randomNames[i] = randomIndexOrAlias();
}
return indices;
return randomNames;
}

private String[] randomUniqueIndicesOrAliases() {
String[] uniqueIndices = randomUniqueIndices();
String[] indices = new String[uniqueIndices.length];
String[] randomNames = new String[uniqueIndices.length];
int i = 0;
for (String index : uniqueIndices) {
indices[i++] = randomBoolean() ? index + "-alias" : index;
randomNames[i++] = randomBoolean() ? index + "-alias" : index;
}
return indices;
return randomNames;
}

private String[] randomUniqueIndices() {
Expand Down Expand Up @@ -776,8 +776,8 @@ synchronized List<TransportRequest> consumeRequests(String action) {
return requests.remove(action);
}

synchronized void interceptTransportActions(String... actions) {
Collections.addAll(this.actions, actions);
synchronized void interceptTransportActions(String... transportActions) {
Collections.addAll(this.actions, transportActions);
}

synchronized void clearInterceptedActions() {
Expand Down
6 changes: 3 additions & 3 deletions ...s/src/test/java/org/elasticsearch/xpack/analytics/topmetrics/InternalTopMetricsTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,7 @@ private InternalTopMetrics createTestInstance(
@Override
protected InternalTopMetrics mutateInstance(InternalTopMetrics instance) throws IOException {
String name = instance.getName();
SortOrder sortOrder = instance.getSortOrder();
SortOrder instanceSortOrder = instance.getSortOrder();
List<String> metricNames = instance.getMetricNames();
int size = instance.getSize();
List<InternalTopMetrics.TopMetric> topMetrics = instance.getTopMetrics();
Expand All @@ -346,7 +346,7 @@ protected InternalTopMetrics mutateInstance(InternalTopMetrics instance) throws
name = randomAlphaOfLength(6);
break;
case 1:
sortOrder = sortOrder == SortOrder.ASC ? SortOrder.DESC : SortOrder.ASC;
instanceSortOrder = instanceSortOrder == SortOrder.ASC ? SortOrder.DESC : SortOrder.ASC;
Collections.reverse(topMetrics);
break;
case 2:
Expand All @@ -372,7 +372,7 @@ protected InternalTopMetrics mutateInstance(InternalTopMetrics instance) throws
default:
throw new IllegalArgumentException("bad mutation");
}
return new InternalTopMetrics(name, sortOrder, metricNames, size, topMetrics, instance.getMetadata());
return new InternalTopMetrics(name, instanceSortOrder, metricNames, size, topMetrics, instance.getMetadata());
}

/**
Expand Down
18 changes: 9 additions & 9 deletions x-pack/plugin/core/src/main/java/org/elasticsearch/license/License.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -555,11 +555,11 @@ public XContentBuilder toInnerXContent(XContentBuilder builder, Params params) t
builder.humanReadable(true);
}
}
final int version;
final int licenseVersion;
if (params.param(LICENSE_VERSION_MODE) != null && restViewMode) {
version = Integer.parseInt(params.param(LICENSE_VERSION_MODE));
licenseVersion = Integer.parseInt(params.param(LICENSE_VERSION_MODE));
} else {
version = this.version;
licenseVersion = this.version;
}
if (restViewMode) {
builder.field(Fields.STATUS, status().label());
Expand All @@ -569,19 +569,19 @@ public XContentBuilder toInnerXContent(XContentBuilder builder, Params params) t
final String bwcType = hideEnterprise && "enterprise".equals(type) ? "platinum" : type;
builder.field(Fields.TYPE, bwcType);

if (version == VERSION_START) {
if (licenseVersion == VERSION_START) {
builder.field(Fields.SUBSCRIPTION_TYPE, subscriptionType);
}
builder.timeField(Fields.ISSUE_DATE_IN_MILLIS, Fields.ISSUE_DATE, issueDate);
if (version == VERSION_START) {
if (licenseVersion == VERSION_START) {
builder.field(Fields.FEATURE, feature);
}

if (expiryDate != LicenseService.BASIC_SELF_GENERATED_LICENSE_EXPIRATION_MILLIS) {
builder.timeField(Fields.EXPIRY_DATE_IN_MILLIS, Fields.EXPIRY_DATE, expiryDate);
}

if (version >= VERSION_ENTERPRISE) {
if (licenseVersion >= VERSION_ENTERPRISE) {
builder.field(Fields.MAX_NODES, maxNodes == -1 ? null : maxNodes);
builder.field(Fields.MAX_RESOURCE_UNITS, maxResourceUnits == -1 ? null : maxResourceUnits);
} else if (hideEnterprise && maxNodes == -1) {
Expand All @@ -598,7 +598,7 @@ public XContentBuilder toInnerXContent(XContentBuilder builder, Params params) t
if (restViewMode) {
builder.humanReadable(previouslyHumanReadable);
}
if (version >= VERSION_START_DATE) {
if (licenseVersion >= VERSION_START_DATE) {
builder.timeField(Fields.START_DATE_IN_MILLIS, Fields.START_DATE, startDate);
}
return builder;
Expand Down Expand Up @@ -921,7 +921,7 @@ public Builder startDate(long startDate) {
return this;
}

public Builder fromLicenseSpec(License license, String signature) {
public Builder fromLicenseSpec(License license, String licenseSignature) {
return uid(license.uid()).version(license.version())
.issuedTo(license.issuedTo())
.issueDate(license.issueDate())
Expand All @@ -933,7 +933,7 @@ public Builder fromLicenseSpec(License license, String signature) {
.maxResourceUnits(license.maxResourceUnits())
.expiryDate(license.expiryDate())
.issuer(license.issuer())
.signature(signature);
.signature(licenseSignature);
}

/**
Expand Down
10 changes: 5 additions & 5 deletions x-pack/plugin/core/src/main/java/org/elasticsearch/license/LicenseService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,8 @@ public class LicenseService extends AbstractLifecycleComponent implements Cluste
/**
* Currently active license
*/
private final AtomicReference<License> currentLicense = new AtomicReference<>();
private SchedulerEngine scheduler;
private final AtomicReference<License> currentLicenseHolder = new AtomicReference<>();
private final SchedulerEngine scheduler;
private final Clock clock;

/**
Expand Down Expand Up @@ -447,7 +447,7 @@ protected void doStop() throws ElasticsearchException {
clusterService.removeListener(this);
scheduler.stop();
// clear current license
currentLicense.set(null);
currentLicenseHolder.set(null);
}

@Override
Expand Down Expand Up @@ -567,9 +567,9 @@ private void onUpdate(final LicensesMetadata currentLicensesMetadata) {
// license can be null if the trial license is yet to be auto-generated
// in this case, it is a no-op
if (license != null) {
final License previousLicense = currentLicense.get();
final License previousLicense = currentLicenseHolder.get();
if (license.equals(previousLicense) == false) {
currentLicense.set(license);
currentLicenseHolder.set(license);
license.setOperationModeFileWatcher(operationModeFileWatcher);
scheduler.add(new SchedulerEngine.Job(LICENSE_JOB, nextLicenseCheck(license)));
for (ExpirationCallback expirationCallback : expirationCallbacks) {
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugin/core/src/main/java/org/elasticsearch/license/Licensing.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ public Licensing(Settings settings) {

@Override
public List<RestHandler> getRestHandlers(
Settings settings,
Settings unused,
RestController restController,
ClusterSettings clusterSettings,
IndexScopedSettings indexScopedSettings,
Expand Down
6 changes: 3 additions & 3 deletions x-pack/plugin/core/src/main/java/org/elasticsearch/license/StartBasicClusterTask.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,9 @@ public ClusterState execute(ClusterState currentState) throws Exception {
if (shouldGenerateNewBasicLicense(currentLicense)) {
License selfGeneratedLicense = generateBasicLicense(currentState);
if (request.isAcknowledged() == false && currentLicense != null) {
Map<String, String[]> ackMessages = LicenseService.getAckMessages(selfGeneratedLicense, currentLicense);
if (ackMessages.isEmpty() == false) {
this.ackMessages.set(ackMessages);
Map<String, String[]> ackMessageMap = LicenseService.getAckMessages(selfGeneratedLicense, currentLicense);
if (ackMessageMap.isEmpty() == false) {
this.ackMessages.set(ackMessageMap);
return currentState;
}
}
Expand Down
Loading

0 comments on commit 6fc8166

Please sign in to comment.