hsheinblatt commented on code in PR #1361: URL: https://github.com/apache/knox/pull/1361#discussion_r3898815619
########## gateway-server/src/main/java/org/apache/knox/gateway/services/knoxidf/delegation/DelegationPolicyDatabase.java: ########## @@ -0,0 +1,426 @@ +/* + * 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 + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * 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.knox.gateway.services.knoxidf.delegation; + +import org.apache.commons.io.IOUtils; +import org.apache.knox.gateway.database.DatabaseType; +import org.apache.knox.gateway.database.JDBCUtils; +import org.apache.knox.gateway.database.KnoxDatabase; + +import javax.sql.DataSource; +import java.io.InputStream; +import java.sql.Connection; +import java.sql.PreparedStatement; +import java.sql.ResultSet; +import java.sql.SQLException; +import java.sql.Timestamp; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.Set; +import java.util.UUID; + +import static java.nio.charset.StandardCharsets.UTF_8; + +/** + * JDBC helper for the five DELEGATION_POLICIES tables. + * All SQL uses {@link PreparedStatement} with {@code ?} parameters only. + * Each public method manages its own {@link Connection} and, for multi-table writes, + * its own transaction boundaries (setAutoCommit / commit / rollback). + */ +class DelegationPolicyDatabase extends KnoxDatabase { + + static final String CORE_TABLE = "DELEGATION_POLICIES"; + + private static final String INSERT_REGISTRATION_SQL = + "INSERT INTO " + CORE_TABLE + + " (registration_id, actor_authority, actor_id, name, status, token_ttl_sec, " + + "description, created_by, created_at, updated_at, allow_headless_exchange) " + + "VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)"; + + private static final String UPDATE_CORE_SQL = + "UPDATE " + CORE_TABLE + " SET " + + "actor_authority = ?, actor_id = ?, name = ?, status = ?, token_ttl_sec = ?, " + + "description = ?, created_by = ?, created_at = ?, updated_at = ?, " + + "allow_headless_exchange = ? " + + "WHERE registration_id = ?"; + + private static final String DELETE_REGISTRATION_SQL = + "DELETE FROM " + CORE_TABLE + " WHERE registration_id = ?"; + + private static final String SELECT_BY_ID_SQL = + "SELECT registration_id, actor_authority, actor_id, name, status, token_ttl_sec, " + + "description, created_by, created_at, updated_at, allow_headless_exchange " + + "FROM " + CORE_TABLE + " WHERE registration_id = ?"; + + private static final String SELECT_BY_ACTOR_SQL = + "SELECT registration_id, actor_authority, actor_id, name, status, token_ttl_sec, " + + "description, created_by, created_at, updated_at, allow_headless_exchange " + + "FROM " + CORE_TABLE + " WHERE actor_authority = ? AND actor_id = ?"; + + private static final String SELECT_ALL_BASE_SQL = + "SELECT registration_id, actor_authority, actor_id, name, status, token_ttl_sec, " + + "description, created_by, created_at, updated_at, allow_headless_exchange " + + "FROM " + CORE_TABLE; + + // Built at construction time with limit+1 baked in as an integer literal (Derby does not + // support ? parameters in FETCH FIRST n ROWS ONLY). Fetching one extra row lets selectAll() + // detect truncation without a second COUNT query. + private final int listMaxTotal; + private final int listMaxPerAuthority; + private final String selectAllSql; + private final String selectAllFilteredSql; + + private static final String INSERT_USER_SQL = + "INSERT INTO DELEGATION_POLICY_USERS (registration_id, username) VALUES (?, ?)"; + + private static final String INSERT_GROUP_SQL = + "INSERT INTO DELEGATION_POLICY_GROUPS (registration_id, group_name) VALUES (?, ?)"; + + private static final String INSERT_RESOURCE_SQL = + "INSERT INTO DELEGATION_POLICY_RESOURCES (registration_id, resource_uri) VALUES (?, ?)"; + + private static final String INSERT_SCOPE_SQL = + "INSERT INTO DELEGATION_POLICY_RESOURCE_SCOPES (registration_id, resource_uri, scope) VALUES (?, ?, ?)"; + + private static final String SELECT_USERS_SQL = + "SELECT username FROM DELEGATION_POLICY_USERS WHERE registration_id = ?"; + + private static final String SELECT_GROUPS_SQL = + "SELECT group_name FROM DELEGATION_POLICY_GROUPS WHERE registration_id = ?"; + + private static final String SELECT_RESOURCES_SQL = + "SELECT resource_uri FROM DELEGATION_POLICY_RESOURCES WHERE registration_id = ?"; + + private static final String SELECT_SCOPES_SQL = + "SELECT scope FROM DELEGATION_POLICY_RESOURCE_SCOPES WHERE registration_id = ? AND resource_uri = ?"; + + private static final String DELETE_USERS_SQL = + "DELETE FROM DELEGATION_POLICY_USERS WHERE registration_id = ?"; + + private static final String DELETE_GROUPS_SQL = + "DELETE FROM DELEGATION_POLICY_GROUPS WHERE registration_id = ?"; + + private static final String DELETE_RESOURCES_SQL = + "DELETE FROM DELEGATION_POLICY_RESOURCES WHERE registration_id = ?"; + + DelegationPolicyDatabase(DataSource dataSource, String dbType, int listMaxTotal, int listMaxPerAuthority) throws Exception { + super(dataSource); + this.listMaxTotal = listMaxTotal; + this.listMaxPerAuthority = listMaxPerAuthority; + this.selectAllSql = SELECT_ALL_BASE_SQL + " FETCH FIRST " + (listMaxTotal + 1) + " ROWS ONLY"; + this.selectAllFilteredSql = SELECT_ALL_BASE_SQL + " WHERE actor_authority = ? FETCH FIRST " + (listMaxPerAuthority + 1) + " ROWS ONLY"; Review Comment: My opinion is that we should not add an order by. That would have performance implications for every query in normal operation. The case in question is already an error case: we've hit the scalability limit of the database and it's no longer working correctly. Clients have a flag so they can log the condition, but the administrator has to fix it. More resources can be assigned and the limit raised, rows could be cleaned up to free up capacity, but the system isn't working anymore. It seemed best to truncate results with a flag rather than throw an exception, but the client has no graceful recovery option here. There's no way to retrieve the missing rows even if there were an order by, and the data returned is incorrect because it is incomplete either way. The design could be changed to use paging instead, but would similarly still have capacity limits that could still be reached and cause error conditions. -- 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]
