xiangfu0 commented on code in PR #18411: URL: https://github.com/apache/pinot/pull/18411#discussion_r3201306563
########## pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/DeprecatedTableConfigValidationUtils.java: ########## @@ -0,0 +1,515 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pinot.controller.api.resources; + +import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.databind.JsonNode; +import java.lang.reflect.Method; +import java.lang.reflect.Modifier; +import java.lang.reflect.ParameterizedType; +import java.lang.reflect.Type; +import java.math.BigDecimal; +import java.math.BigInteger; +import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Set; +import javax.annotation.Nullable; +import org.apache.pinot.common.version.PinotVersion; +import org.apache.pinot.spi.config.BaseJsonConfig; +import org.apache.pinot.spi.config.DeprecatedConfig; +import org.apache.pinot.spi.config.table.TableConfig; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + + +/// Validates raw table-config JSON for deprecated properties on create and update paths. +/// +/// Rules are derived at class-load time from [DeprecatedConfig] annotations on getters reachable from [TableConfig]. +/// The current soft-launch policy reports every parseable annotation `since` value as [Severity#WARNING] so existing +/// callers continue to work while seeing migration guidance. Only an unparseable annotation `since` value classifies +/// as [Severity#ERROR], since that case is a code-side bug rather than user-supplied data. +/// +/// The validator operates on raw JSON rather than the deserialized [TableConfig] so it can detect explicitly +/// provided deprecated keys even when they carry default or false-y values that Jackson would otherwise elide. +public final class DeprecatedTableConfigValidationUtils { + private DeprecatedTableConfigValidationUtils() { + } + + private static final Logger LOGGER = LoggerFactory.getLogger(DeprecatedTableConfigValidationUtils.class); + private static final String WILDCARD = "*"; + private static final String FIELD_CONFIG_INDEX_TYPES_KEY = "indexTypes"; + private static final List<String> FIELD_CONFIG_INDEX_TYPE_PATH = + List.of(TableConfig.FIELD_CONFIG_LIST_KEY, WILDCARD, "indexType"); + // CURRENT_MAJOR_MINOR must be initialized before RULES because discoverRules() -> classifySeverity() reads it. + private static final String CURRENT_MAJOR_MINOR = currentMajorMinor(); + private static final List<DeprecatedConfigRule> RULES = discoverRules(); + + public enum Severity { + WARNING, ERROR + } + + /// Result of validating a table-config JSON, exposing errors and warnings separately so callers can decide + /// whether to reject the request or surface non-fatal warnings to the user. + public static final class Result { + private final List<String> _errors; + private final List<String> _warnings; + + private Result(List<String> errors, List<String> warnings) { + _errors = List.copyOf(errors); + _warnings = List.copyOf(warnings); + } + + public List<String> getErrors() { + return _errors; + } + + public List<String> getWarnings() { + return _warnings; + } + + public boolean hasErrors() { + return !_errors.isEmpty(); + } + + public boolean hasWarnings() { + return !_warnings.isEmpty(); + } + } + + /// Validates a table-config JSON document. When `oldTableConfigJson` is non-null, only paths whose value newly + /// appears in `newTableConfigJson` or whose value differs from the corresponding path in `oldTableConfigJson` are + /// reported, so re-submitting an unchanged legacy field on an update is a no-op. When `oldTableConfigJson` is null + /// (creation path), every present deprecated path is reported. + /// + /// @param newTableConfigJson the incoming table config to validate; must not be `null` + /// @param oldTableConfigJson the currently-stored table config to diff against, or `null` for create paths + /// @param rootPathPrefix optional prefix for emitted paths (e.g. `"realtime"` when validating a sub-section) + public static Result validate(JsonNode newTableConfigJson, @Nullable JsonNode oldTableConfigJson, + @Nullable String rootPathPrefix) { + Objects.requireNonNull(newTableConfigJson, "newTableConfigJson"); + List<String> errors = new ArrayList<>(); + List<String> warnings = new ArrayList<>(); + String prefix = rootPathPrefix == null ? "" : rootPathPrefix; + for (DeprecatedConfigRule rule : RULES) { + rule.collect(newTableConfigJson, oldTableConfigJson, prefix, errors, warnings); + } + return new Result(errors, warnings); + } + + /// Validates a freshly-submitted table config (no prior stored value). On any error the method throws + /// [IllegalArgumentException]; warnings are returned so the caller can surface them in the response. + public static List<String> validateOnCreate(JsonNode newTableConfigJson, @Nullable String rootPathPrefix) { + Result result = validate(newTableConfigJson, null, rootPathPrefix); + if (result.hasErrors()) { + throw new IllegalArgumentException("Deprecated table config properties are not allowed on table creation: " + + String.join("; ", result.getErrors())); + } + if (result.hasWarnings()) { + LOGGER.warn("Deprecated table config properties on creation: {}", String.join("; ", result.getWarnings())); + } + return result.getWarnings(); + } + + /// Validates an updated table config against its currently-stored counterpart. Only newly-introduced or + /// value-changed deprecated paths are reported, so legacy values that were already present do not block updates. + /// On any error the method throws [IllegalArgumentException]; warnings are returned for the caller to surface. + /// + /// Callers must ensure the table exists before invoking this method; pass the stored config JSON (never `null`) + /// for `oldTableConfigJson`. For paths where no stored counterpart exists, use [#validateOnCreate] instead. + public static List<String> validateOnUpdate(JsonNode newTableConfigJson, JsonNode oldTableConfigJson, + @Nullable String rootPathPrefix) { + Objects.requireNonNull(oldTableConfigJson, "oldTableConfigJson; use validateOnCreate for create paths"); + Result result = validate(newTableConfigJson, oldTableConfigJson, rootPathPrefix); + if (result.hasErrors()) { + throw new IllegalArgumentException("Newly introduced deprecated table config properties are not allowed: " + + String.join("; ", result.getErrors())); + } + if (result.hasWarnings()) { + LOGGER.warn("Newly introduced deprecated table config properties on update: {}", + String.join("; ", result.getWarnings())); + } + return result.getWarnings(); + } + + static List<DeprecatedConfigRule> rulesForTesting() { + return Collections.unmodifiableList(RULES); + } + + private static List<DeprecatedConfigRule> discoverRules() { + List<DeprecatedConfigRule> rules = new ArrayList<>(); + walk(TableConfig.class, new ArrayList<>(), new HashSet<>(), rules); + return Collections.unmodifiableList(rules); + } + + private static void walk(Type type, List<String> currentPath, Set<Class<?>> visiting, + List<DeprecatedConfigRule> rules) { + Class<?> rawClass = rawClass(type); + if (rawClass == null) { + return; + } + if (Map.class.isAssignableFrom(rawClass)) { + Type valueType = typeArg(type, 1); + if (valueType != null) { + currentPath.add(WILDCARD); + walk(valueType, currentPath, visiting, rules); + currentPath.remove(currentPath.size() - 1); + } + return; + } + if (Collection.class.isAssignableFrom(rawClass)) { + Type elemType = typeArg(type, 0); + if (elemType != null) { + currentPath.add(WILDCARD); + walk(elemType, currentPath, visiting, rules); + currentPath.remove(currentPath.size() - 1); + } + return; + } + if (rawClass.isArray()) { + currentPath.add(WILDCARD); + walk(rawClass.getComponentType(), currentPath, visiting, rules); + currentPath.remove(currentPath.size() - 1); + return; + } + if (!isConfigBean(rawClass) || !visiting.add(rawClass)) { + return; + } + try { + for (Method method : rawClass.getMethods()) { + if (!isJsonAccessor(method)) { + continue; + } + String propertyName = jsonPropertyName(method); + if (propertyName == null) { + continue; + } + currentPath.add(propertyName); + DeprecatedConfig anno = method.getAnnotation(DeprecatedConfig.class); Review Comment: This validator only creates deprecated-key rules from getters annotated with `@DeprecatedConfig`. In this branch, `SegmentsValidationAndRetentionConfig#getSegmentPushType()` and `#getSegmentPushFrequency()` are still plain `@Deprecated` getters, so create/update requests can keep sending `segmentsConfig.segmentPushType` / `segmentsConfig.segmentPushFrequency` with no warning or rejection even though the PR documents those keys as migrated to `ingestionConfig.batchIngestionConfig.*`. The parameterized `rulesForTesting()` coverage also will not catch this omission because missing annotations simply never enter the discovered rule list. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
