rmetzger commented on code in PR #38:
URL:
https://github.com/apache/flink-connector-http/pull/38#discussion_r3078705063
##########
flink-connector-http/src/main/java/org/apache/flink/connector/http/table/lookup/querycreators/GenericJsonAndUrlQueryCreatorFactory.java:
##########
@@ -136,7 +162,138 @@ public Set<ConfigOption<?>> requiredOptions() {
@Override
public Set<ConfigOption<?>> optionalOptions() {
- return Set.of(REQUEST_QUERY_PARAM_FIELDS, REQUEST_URL_MAP,
REQUEST_BODY_TEMPLATE);
+ return Set.of(
+ REQUEST_QUERY_PARAM_FIELDS,
+ REQUEST_QUERY_PARAM_FIELDS_WITH_KEY,
+ REQUEST_URL_MAP,
+ REQUEST_BODY_TEMPLATE);
+ }
+
+ /**
+ * Validates the query parameter map to ensure all constraints are met.
+ *
+ * <ul>
+ * <li>Column names (keys) are not null or empty
+ * <li>Query param keys (values) are not null or empty
+ * <li>Query param keys do not conflict with old
REQUEST_QUERY_PARAM_FIELDS list
+ * <li>Query param keys are unique (no duplicates)
+ * <li>Column names in new map do not conflict with column names in old
list
+ * </ul>
+ *
+ * @param queryParamMap the map to validate
+ * @param readableConfig the configuration to check for conflicts
+ * @throws IllegalArgumentException if validation fails
+ */
+ private void validateQueryParamMap(
+ Map<String, String> queryParamMap, ReadableConfig readableConfig) {
+ if (queryParamMap == null || queryParamMap.isEmpty()) {
+ return; // Empty map is valid
+ }
+
+ // Get the list format for conflict checking
+ final List<String> queryParamFields =
readableConfig.get(REQUEST_QUERY_PARAM_FIELDS);
+
+ // Track seen query param keys to detect duplicates
+ Set<String> seenQueryParamKeys = new HashSet<>();
+
+ for (Map.Entry<String, String> entry : queryParamMap.entrySet()) {
+ String columnName = entry.getKey();
+ String queryParamKey = entry.getValue();
+
+ // Validate column name (map key)
+ if (columnName == null || columnName.trim().isEmpty()) {
+ throw new IllegalArgumentException(
+ "Column name in "
+ + REQUEST_QUERY_PARAM_FIELDS_WITH_KEY.key()
+ + " cannot be null or empty");
+ }
+
+ // Validate query param key (map value)
+ if (queryParamKey == null || queryParamKey.trim().isEmpty()) {
+ throw new IllegalArgumentException(
+ "Query parameter key for column '"
+ + columnName
+ + "' in "
+ + REQUEST_QUERY_PARAM_FIELDS_WITH_KEY.key()
+ + " cannot be null or empty");
+ }
+
+ // Check for duplicate query param keys
+ if (seenQueryParamKeys.contains(queryParamKey)) {
+ throw new IllegalArgumentException(
+ "Duplicate query parameter key '"
+ + queryParamKey
+ + "' in "
+ + REQUEST_QUERY_PARAM_FIELDS_WITH_KEY.key()
+ + ". Each query parameter key must be
unique.");
+ }
+ seenQueryParamKeys.add(queryParamKey);
+
+ // Check for conflicts with list format - both query param keys
and column names
+ if (!queryParamFields.isEmpty()
+ && (queryParamFields.contains(queryParamKey)
+ || queryParamFields.contains(columnName))) {
+ throw new IllegalArgumentException(
+ (queryParamFields.contains(queryParamKey)
+ ? "Query parameter key '" +
queryParamKey
+ : "Column name '" + columnName)
+ + "' in "
+ + REQUEST_QUERY_PARAM_FIELDS_WITH_KEY.key()
+ + " conflicts with existing columns defined in
"
+ + REQUEST_QUERY_PARAM_FIELDS.key());
+ }
+ }
+ }
+
+ /**
+ * Validates that configuration options are appropriate for the HTTP
method.
+ *
+ * <p>Rules:
+ *
+ * <ul>
+ * <li>Query parameter configs can only be used with GET
+ * <li>Body template can only be used with POST/PUT
+ * </ul>
+ *
+ * @param httpMethod the HTTP method (GET, POST, PUT)
+ * @param readableConfig the configuration to validate
+ * @throws IllegalArgumentException if configuration is invalid for the
HTTP method
+ */
+ private void validateConfigurationForHttpMethod(
+ String httpMethod, ReadableConfig readableConfig) {
+ boolean hasQueryParams =
!readableConfig.get(REQUEST_QUERY_PARAM_FIELDS).isEmpty();
+ boolean hasQueryParamsWithKey =
+
readableConfig.getOptional(REQUEST_QUERY_PARAM_FIELDS_WITH_KEY).isPresent();
+ boolean hasBodyTemplate =
+ readableConfig.getOptional(REQUEST_BODY_TEMPLATE).isPresent()
+ &&
!readableConfig.get(REQUEST_BODY_TEMPLATE).trim().isEmpty();
+
+ if (httpMethod.equalsIgnoreCase("GET")) {
+ if (hasBodyTemplate) {
+ throw new IllegalArgumentException(
+ "Body template configuration ("
+ + REQUEST_BODY_TEMPLATE.key()
+ + ") cannot be used with GET method. "
+ + "For GET requests, use "
+ + REQUEST_QUERY_PARAM_FIELDS.key()
+ + " or "
+ + REQUEST_QUERY_PARAM_FIELDS_WITH_KEY.key()
+ + " instead.");
+ }
+ } else {
+ // POST/PUT
+ if (hasQueryParams || hasQueryParamsWithKey) {
+ throw new IllegalArgumentException(
+ "Query parameter configuration ("
+ + REQUEST_QUERY_PARAM_FIELDS.key()
+ + " or "
+ + REQUEST_QUERY_PARAM_FIELDS_WITH_KEY.key()
+ + ") can only be used with GET method. "
+ + "For POST/PUT requests, use "
+ + REQUEST_BODY_TEMPLATE.key()
+ + " instead.");
+ }
Review Comment:
My AI tells me this:
The new `validateConfigurationForHttpMethod()` throws
`IllegalArgumentException` when `http.request.query-param-fields` is set with
POST or PUT. Previously, this was valid — query param fields were used to
populate `bodyBasedUrlQueryParams` for the request body. Evidence:
- The old `getConfiguration()` test helper set `REQUEST_QUERY_PARAM_FIELDS`
for ALL operations including POST/PUT
- The old `validateCreatedQueryForPutAndPost` asserted
`hasBodyBasedUrlQueryParameters().isTrue()`
**Any existing user** with a config like `'lookup-method' = 'POST',
'http.request.query-param-fields' = 'id;name'` will get a runtime error after
upgrading with no migration path other than rewriting to use `body-template`.
**Recommendation:** Either remove the validation to preserve backward
compatibility, or document this as an intentional breaking change with
migration guidance.
--
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]
