Skip to content

Commit

Permalink
Checkstyle shadows vars pt8 (#81147)
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:    * Remove unused bits and pieces from
`XPackClientPlugin`.    * 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 8b07747 commit c1efe47
Show file tree
Hide file tree
Showing 70 changed files with 269 additions and 264 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 @@ -278,31 +278,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).id("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).id("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, "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 @@ -342,36 +342,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, 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, 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 @@ -385,9 +385,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 @@ -412,9 +412,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 @@ -670,21 +670,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 @@ -771,8 +771,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 @@ -336,7 +336,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 @@ -345,7 +345,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 @@ -371,7 +371,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 @@ -575,31 +575,31 @@ 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());
}
builder.field(Fields.UID, uid);
final String bwcType = hideEnterprise && LicenseType.isEnterprise(type) ? LicenseType.PLATINUM.getTypeName() : 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 @@ -616,7 +616,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 @@ -939,7 +939,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 @@ -951,7 +951,7 @@ public Builder fromLicenseSpec(License license, String signature) {
.maxResourceUnits(license.maxResourceUnits())
.expiryDate(license.expiryDate())
.issuer(license.issuer())
.signature(signature);
.signature(licenseSignature);
}

/**
Expand Down
8 changes: 4 additions & 4 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,7 +109,7 @@ public class LicenseService extends AbstractLifecycleComponent implements Cluste
/**
* Currently active license
*/
private final AtomicReference<License> currentLicense = new AtomicReference<>();
private final AtomicReference<License> currentLicenseHolder = new AtomicReference<>();
private final SchedulerEngine scheduler;
private final Clock clock;

Expand Down Expand Up @@ -438,7 +438,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 @@ -558,9 +558,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 c1efe47

Please sign in to comment.