Copilot commented on code in PR #6249:
URL: https://github.com/apache/shenyu/pull/6249#discussion_r2587985949
##########
shenyu-admin/src/main/java/org/apache/shenyu/admin/service/support/AiProxyRealKeyResolver.java:
##########
@@ -185,10 +194,66 @@ private String findFirstApiKey(final JsonNode node) {
return null;
}
+ /**
+ * Batch resolve real api keys.
+ *
+ * @param selectorIds set of selector ids
+ * @return map of selectorId -> real api key
+ */
+ public Map<String, String> resolveRealKeys(final java.util.Set<String>
selectorIds) {
+ if (Objects.isNull(selectorIds) || selectorIds.isEmpty()) {
+ return java.util.Collections.emptyMap();
+ }
+ final Map<String, String> result = new java.util.HashMap<>();
+ final java.util.Set<String> missing = new java.util.HashSet<>();
+
+ for (String id : selectorIds) {
+ AtomicReference<String> ref = cache.get(id);
+ if (Objects.nonNull(ref)) {
+ result.put(id, ref.get());
+ } else {
+ missing.add(id);
+ }
+ }
+
+ if (!missing.isEmpty()) {
+ java.util.List<SelectorDO> selectors =
selectorMapper.selectByIdSet(missing);
+ if (Objects.nonNull(selectors)) {
+ for (SelectorDO selector : selectors) {
+ String apiKey = extractApiKey(selector.getHandle());
+ cache.put(selector.getId(), new AtomicReference<>(apiKey));
+ result.put(selector.getId(), apiKey);
+ }
+ }
+ // For IDs that were not found in DB, cache them as null to avoid
repeated DB
+ // hits?
+ // Current single-resolve logic returns Optional.empty() but
doesn't cache null
+ // if exception occurs (my fix).
+ // But if selector is not found, doResolve returns null.
+ // My previous fix in doResolve:
+ // if (Objects.isNull(selector) ... ) return null;
+ // And resolveRealKey:
+ // .whenComplete((val, ex) -> { ... cache.put(id, new
AtomicReference<>(val));
+ // ... })
+ // So nulls ARE cached.
+
+ // So for missing IDs in DB, we should also cache null.
Review Comment:
Long inline comments explaining implementation details should be removed.
These comments (lines 228-238) appear to be developer notes/thought process
rather than necessary code documentation. Consider removing them or converting
to a brief comment if the caching behavior needs clarification.
```suggestion
// Cache null for IDs not found in DB to avoid repeated DB hits.
```
##########
shenyu-admin/src/main/java/org/apache/shenyu/admin/service/support/AiProxyRealKeyResolver.java:
##########
@@ -185,10 +194,66 @@ private String findFirstApiKey(final JsonNode node) {
return null;
}
+ /**
+ * Batch resolve real api keys.
+ *
+ * @param selectorIds set of selector ids
+ * @return map of selectorId -> real api key
+ */
+ public Map<String, String> resolveRealKeys(final java.util.Set<String>
selectorIds) {
+ if (Objects.isNull(selectorIds) || selectorIds.isEmpty()) {
+ return java.util.Collections.emptyMap();
+ }
+ final Map<String, String> result = new java.util.HashMap<>();
+ final java.util.Set<String> missing = new java.util.HashSet<>();
+
+ for (String id : selectorIds) {
+ AtomicReference<String> ref = cache.get(id);
+ if (Objects.nonNull(ref)) {
+ result.put(id, ref.get());
+ } else {
+ missing.add(id);
+ }
+ }
+
+ if (!missing.isEmpty()) {
+ java.util.List<SelectorDO> selectors =
selectorMapper.selectByIdSet(missing);
+ if (Objects.nonNull(selectors)) {
+ for (SelectorDO selector : selectors) {
+ String apiKey = extractApiKey(selector.getHandle());
+ cache.put(selector.getId(), new AtomicReference<>(apiKey));
+ result.put(selector.getId(), apiKey);
+ }
+ }
+ // For IDs that were not found in DB, cache them as null to avoid
repeated DB
+ // hits?
+ // Current single-resolve logic returns Optional.empty() but
doesn't cache null
+ // if exception occurs (my fix).
+ // But if selector is not found, doResolve returns null.
+ // My previous fix in doResolve:
+ // if (Objects.isNull(selector) ... ) return null;
+ // And resolveRealKey:
+ // .whenComplete((val, ex) -> { ... cache.put(id, new
AtomicReference<>(val));
+ // ... })
+ // So nulls ARE cached.
+
+ // So for missing IDs in DB, we should also cache null.
+ for (String id : missing) {
+ if (!result.containsKey(id)) {
+ // If not found in DB or handle is invalid, it wasn't
added to result.
+ // We should add it as null to result and cache.
+ cache.put(id, new AtomicReference<>(null));
+ result.put(id, null);
+ }
+ }
+ }
+ return result;
+ }
Review Comment:
The new `resolveRealKeys` batch method lacks test coverage. Add unit tests
to verify: 1) proper cache hit behavior, 2) batch database query for cache
misses, 3) null caching for non-existent selectors, 4) handling of empty or
null input sets.
##########
shenyu-admin/src/main/java/org/apache/shenyu/admin/service/support/AiProxyRealKeyResolver.java:
##########
@@ -57,13 +60,18 @@ public class AiProxyRealKeyResolver {
/** selectorId -> in-flight resolving future. */
private final Map<String, CompletableFuture<String>> inFlight = new
ConcurrentHashMap<>();
+ private final ExecutorService executorService;
+
public AiProxyRealKeyResolver(final SelectorMapper selectorMapper, final
ObjectMapper objectMapper) {
this.selectorMapper = selectorMapper;
this.objectMapper = objectMapper;
+ this.executorService = Executors.newFixedThreadPool(10,
+ ShenyuThreadFactory.create("ai-proxy-key-resolver", false));
Review Comment:
The ExecutorService is never shut down, leading to a potential resource leak
when the application stops. Add a `@PreDestroy` annotated method to properly
shut down the executor service:
```java
@PreDestroy
public void destroy() {
executorService.shutdown();
try {
if (!executorService.awaitTermination(5, TimeUnit.SECONDS)) {
executorService.shutdownNow();
}
} catch (InterruptedException e) {
executorService.shutdownNow();
Thread.currentThread().interrupt();
}
}
```
##########
shenyu-admin/src/main/java/org/apache/shenyu/admin/service/impl/AiProxyConnectionServiceImpl.java:
##########
@@ -39,56 +39,68 @@
*/
@Service
public class AiProxyConnectionServiceImpl implements AiProxyConnectionService {
-
+
private static final Logger LOG =
LoggerFactory.getLogger(AiProxyConnectionServiceImpl.class);
-
+
private final AiProxyRealKeyResolver aiProxyRealKeyResolver;
-
+
private final AiProxyApiKeyMapper aiProxyApiKeyMapper;
-
+
private final ApplicationEventPublisher eventPublisher;
-
+
public AiProxyConnectionServiceImpl(final AiProxyRealKeyResolver
aiProxyRealKeyResolver,
- final AiProxyApiKeyMapper
aiProxyApiKeyMapper,
- final ApplicationEventPublisher
eventPublisher) {
+ final AiProxyApiKeyMapper aiProxyApiKeyMapper,
+ final ApplicationEventPublisher eventPublisher) {
this.aiProxyRealKeyResolver = aiProxyRealKeyResolver;
this.aiProxyApiKeyMapper = aiProxyApiKeyMapper;
this.eventPublisher = eventPublisher;
}
-
+
@Override
public void refreshApiKeysBySelectorId(final String selectorId) {
- // 1. Invalidate resolver cache to ensure next step fetches the new
key from selector's handle
+ // 1. Invalidate resolver cache to ensure next step fetches the new
key from
+ // selector's handle
aiProxyRealKeyResolver.invalidate(selectorId);
LOG.info("[AiProxyConnectionService] invalidated real-key resolver for
selectorId={}", selectorId);
-
+
// 2. Find all proxy api keys associated with this selector
List<ProxyApiKeyDO> keys =
aiProxyApiKeyMapper.selectBySelectorId(selectorId);
if (CollectionUtils.isEmpty(keys)) {
LOG.info("[AiProxyConnectionService] no api keys found for
selectorId={}, skipping refresh", selectorId);
return;
}
-
+
// 3. Convert to ProxyApiKeyData, which will resolve the new realApiKey
List<ProxyApiKeyData> apiKeyDataList = keys.stream()
.map(this::buildData)
.collect(Collectors.toList());
-
+
// 4. Publish an UPDATE event to sync data to gateway
- eventPublisher.publishEvent(new
DataChangedEvent(ConfigGroupEnum.AI_PROXY_API_KEY, DataEventTypeEnum.UPDATE,
apiKeyDataList));
- LOG.info("[AiProxyConnectionService] published UPDATE event for {} api
keys under selectorId={}", apiKeyDataList.size(), selectorId);
+ eventPublisher.publishEvent(
+ new DataChangedEvent(ConfigGroupEnum.AI_PROXY_API_KEY,
DataEventTypeEnum.UPDATE, apiKeyDataList));
+ LOG.info("[AiProxyConnectionService] published UPDATE event for {} api
keys under selectorId={}",
+ apiKeyDataList.size(), selectorId);
}
-
+
private ProxyApiKeyData buildData(final ProxyApiKeyDO apiKeyDO) {
String realApiKey =
aiProxyRealKeyResolver.resolveRealKey(apiKeyDO.getSelectorId()).orElse(null);
+ String normalizedNs = normalizeNamespace(apiKeyDO.getNamespaceId());
ProxyApiKeyData data = new ProxyApiKeyData();
data.setSelectorId(apiKeyDO.getSelectorId());
- data.setNamespaceId(apiKeyDO.getNamespaceId());
+ data.setNamespaceId(normalizedNs);
data.setProxyApiKey(apiKeyDO.getProxyApiKey());
data.setRealApiKey(realApiKey);
data.setEnabled(Boolean.TRUE.equals(apiKeyDO.getEnabled()));
data.setDescription(apiKeyDO.getDescription());
return data;
}
-
-}
\ No newline at end of file
+
+ private String normalizeNamespace(final String namespaceId) {
+ if (org.apache.commons.lang3.StringUtils.isBlank(namespaceId)
+ ||
org.apache.commons.lang3.StringUtils.equalsIgnoreCase(namespaceId, "default")) {
+ return
org.apache.shenyu.common.constant.Constants.SYS_DEFAULT_NAMESPACE_ID;
+ }
+ return namespaceId;
+ }
Review Comment:
[nitpick] The `normalizeNamespace` method is duplicated in both
`AiProxyConnectionServiceImpl` and `AiProxyApiKeyServiceImpl` (line 327).
Consider extracting this to a shared utility method to avoid code duplication
and ensure consistent behavior.
##########
shenyu-admin/src/main/java/org/apache/shenyu/admin/service/impl/AiProxyApiKeyServiceImpl.java:
##########
@@ -131,6 +132,13 @@ public ProxyApiKeyVO findById(final String id) {
return vo;
}
+ @Override
+ public List<ProxyApiKeyVO> findByIds(final List<String> ids) {
+ return mapper.selectByIds(ids).stream()
+ .map(ProxyApiKeyTransfer.INSTANCE::mapToVO)
+ .collect(Collectors.toList());
Review Comment:
[nitpick] The `findByIds` method doesn't populate the `realApiKey` field,
while `findById` does (line 129-130). Consider adding a JavaDoc comment
explaining this difference in behavior, or populate `realApiKey` using the
batch `resolveRealKeys` method for consistency.
```suggestion
List<ProxyApiKeyVO> voList = mapper.selectByIds(ids).stream()
.map(ProxyApiKeyTransfer.INSTANCE::mapToVO)
.collect(Collectors.toList());
// Populate realApiKey for each VO using batch resolver
List<String> selectorIds = voList.stream()
.map(ProxyApiKeyVO::getSelectorId)
.filter(Objects::nonNull)
.collect(Collectors.toList());
if (!selectorIds.isEmpty()) {
// resolveRealKeys returns Map<String, String> mapping
selectorId to realApiKey
java.util.Map<String, String> realKeyMap =
realKeyResolver.resolveRealKeys(selectorIds);
for (ProxyApiKeyVO vo : voList) {
if (vo.getSelectorId() != null) {
vo.setRealApiKey(realKeyMap.get(vo.getSelectorId()));
}
}
}
return voList;
```
--
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]
