Copilot commented on code in PR #3277:
URL: https://github.com/apache/apisix-dashboard/pull/3277#discussion_r2844913705
##########
src/utils/producer.ts:
##########
@@ -64,7 +106,9 @@ export const pipeProduce = (...funcs: ((a: any) =>
unknown)[]) => {
...fs,
produceRmDoubleUnderscoreKeys,
produceTime,
- produceDeepCleanEmptyKeys()
+ producePreservePlugins, // Mark empty plugin configs before cleaning
+ produceDeepCleanEmptyKeys(),
+ produceRemovePreserveMarkers // Restore empty plugin configs after
cleaning
Review Comment:
In `pipeProduce`, the preserve-marker steps are now applied to every
payload. If the intent is only to preserve empty objects, this should be
unnecessary with `emptyObjectsCleaner: false` and increases the chance of
subtle payload mutation for any resource that has a `plugins` field. Suggest
dropping these steps from the default pipeline unless there’s a proven case
where `fast-clean` still drops empty plugin objects with `emptyObjectsCleaner:
false`.
```suggestion
produceDeepCleanEmptyKeys()
```
##########
src/utils/useSearchParams.ts:
##########
@@ -37,7 +37,7 @@ export const useSearchParams = <T extends RouteTreeIds, P
extends object>(
(props: Partial<P>) => {
return navigate({
to: '.',
- search: (prev) => ({ ...prev, ...props }),
+ search: (prev: Record<string, unknown>) => ({ ...prev, ...props }),
Review Comment:
Typing `prev` as `Record<string, unknown>` throws away the route’s inferred
search-param type (and can mask mismatches between `props` and the route’s
search schema). Since this hook already has a generic `P`, it’s safer to type
`prev` as `P` (or `Partial<P>`) so `setParams` stays type-aligned with the
router search object.
```suggestion
search: (prev: P) => ({ ...prev, ...props }),
```
##########
e2e/tests/routes.empty-plugin-config.spec.ts:
##########
@@ -0,0 +1,164 @@
+/**
+ * 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.
+ */
+import { routesPom } from '@e2e/pom/routes';
+import { randomId } from '@e2e/utils/common';
+import { e2eReq } from '@e2e/utils/req';
+import { test } from '@e2e/utils/test';
+import {
+ uiFillMonacoEditor,
+ uiGetMonacoEditor,
+ uiHasToastMsg,
+} from '@e2e/utils/ui';
+import { uiFillUpstreamRequiredFields } from '@e2e/utils/ui/upstreams';
+import { expect } from '@playwright/test';
+
+import { deleteAllRoutes } from '@/apis/routes';
+
+const routeNameWithEmptyPlugin = randomId('test-route-empty-plugin');
+const routeUri = '/test-empty-plugin';
+
+test.beforeAll(async () => {
+ await deleteAllRoutes(e2eReq);
+});
+
+test('should preserve plugin with empty configuration (key-auth) after edit',
async ({
+ page,
+}) => {
+ await routesPom.toIndex(page);
+ await routesPom.isIndexPage(page);
+
+ await test.step('create route with key-auth plugin having configuration',
async () => {
+ await routesPom.getAddRouteBtn(page).click();
Review Comment:
This test covers the “edit plugin config to `{}`” path, but it doesn’t cover
the other reported failure mode: adding a plugin with an empty config on
initial route creation (Issue #3269). Consider extending the test (or adding a
second one) that adds `key-auth` with `{}` during creation and verifies it
persists after save + reload, so the regression is fully covered.
##########
e2e/tests/routes.empty-plugin-config.spec.ts:
##########
@@ -0,0 +1,164 @@
+/**
+ * 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.
+ */
+import { routesPom } from '@e2e/pom/routes';
+import { randomId } from '@e2e/utils/common';
+import { e2eReq } from '@e2e/utils/req';
+import { test } from '@e2e/utils/test';
+import {
+ uiFillMonacoEditor,
+ uiGetMonacoEditor,
+ uiHasToastMsg,
+} from '@e2e/utils/ui';
+import { uiFillUpstreamRequiredFields } from '@e2e/utils/ui/upstreams';
+import { expect } from '@playwright/test';
+
+import { deleteAllRoutes } from '@/apis/routes';
+
+const routeNameWithEmptyPlugin = randomId('test-route-empty-plugin');
+const routeUri = '/test-empty-plugin';
+
+test.beforeAll(async () => {
+ await deleteAllRoutes(e2eReq);
+});
+
+test('should preserve plugin with empty configuration (key-auth) after edit',
async ({
+ page,
+}) => {
+ await routesPom.toIndex(page);
+ await routesPom.isIndexPage(page);
+
+ await test.step('create route with key-auth plugin having configuration',
async () => {
+ await routesPom.getAddRouteBtn(page).click();
+ await routesPom.isAddPage(page);
+
+ // Fill in required fields
+ await page.getByLabel('Name', { exact: true
}).first().fill(routeNameWithEmptyPlugin);
+ await page.getByLabel('URI', { exact: true }).fill(routeUri);
+
+ // Select HTTP method
+ await page.getByRole('textbox', { name: 'HTTP Methods' }).click();
+ await page.getByRole('option', { name: 'GET' }).click();
+
+ // Add upstream nodes
+ const upstreamSection = page.getByRole('group', {
+ name: 'Upstream',
+ exact: true,
+ });
+ await uiFillUpstreamRequiredFields(upstreamSection, {
+ nodes: [
+ { host: 'httpbin.org', port: 80, weight: 1 },
+ { host: 'httpbin.org', port: 80, weight: 1 },
+ ],
+ name: 'test-upstream-empty-plugin',
+ });
+
+ // Add key-auth plugin with some configuration first
+ const selectPluginsBtn = page.getByRole('button', {
+ name: 'Select Plugins',
+ });
+ await selectPluginsBtn.click();
+
+ const selectPluginsDialog = page.getByRole('dialog', {
+ name: 'Select Plugins',
+ });
+ const searchInput = selectPluginsDialog.getByPlaceholder('Search');
+ await searchInput.fill('key-auth');
+
+ await selectPluginsDialog
+ .getByTestId('plugin-key-auth')
+ .getByRole('button', { name: 'Add' })
+ .click();
+
+ // Add plugin with initial configuration
+ const addPluginDialog = page.getByRole('dialog', { name: 'Add Plugin'
});
+ const pluginEditor = await uiGetMonacoEditor(page, addPluginDialog);
+ await uiFillMonacoEditor(page, pluginEditor, '{"hide_credentials":
true}');
+ await addPluginDialog.getByRole('button', { name: 'Add' }).click();
+ await expect(addPluginDialog).toBeHidden();
+
+ // Verify the plugin was added
+ const pluginsSection = page.getByRole('group', { name: 'Plugins' });
+ await
expect(pluginsSection.getByTestId('plugin-key-auth')).toBeVisible();
+
+ // Submit the form
+ await routesPom.getAddBtn(page).click();
+ await uiHasToastMsg(page, {
+ hasText: 'Add Route Successfully',
+ });
+ });
Review Comment:
Indentation in this new spec file is inconsistent with other e2e specs (most
use 2-space indentation, e.g. `e2e/tests/routes.crud-required-fields.spec.ts`).
Aligning the indentation here will keep diffs smaller and readability
consistent across the e2e suite.
```suggestion
await deleteAllRoutes(e2eReq);
});
test('should preserve plugin with empty configuration (key-auth) after
edit', async ({
page,
}) => {
await routesPom.toIndex(page);
await routesPom.isIndexPage(page);
await test.step('create route with key-auth plugin having configuration',
async () => {
await routesPom.getAddRouteBtn(page).click();
await routesPom.isAddPage(page);
// Fill in required fields
await page.getByLabel('Name', { exact: true
}).first().fill(routeNameWithEmptyPlugin);
await page.getByLabel('URI', { exact: true }).fill(routeUri);
// Select HTTP method
await page.getByRole('textbox', { name: 'HTTP Methods' }).click();
await page.getByRole('option', { name: 'GET' }).click();
// Add upstream nodes
const upstreamSection = page.getByRole('group', {
name: 'Upstream',
exact: true,
});
await uiFillUpstreamRequiredFields(upstreamSection, {
nodes: [
{ host: 'httpbin.org', port: 80, weight: 1 },
{ host: 'httpbin.org', port: 80, weight: 1 },
],
name: 'test-upstream-empty-plugin',
});
// Add key-auth plugin with some configuration first
const selectPluginsBtn = page.getByRole('button', {
name: 'Select Plugins',
});
await selectPluginsBtn.click();
const selectPluginsDialog = page.getByRole('dialog', {
name: 'Select Plugins',
});
const searchInput = selectPluginsDialog.getByPlaceholder('Search');
await searchInput.fill('key-auth');
await selectPluginsDialog
.getByTestId('plugin-key-auth')
.getByRole('button', { name: 'Add' })
.click();
// Add plugin with initial configuration
const addPluginDialog = page.getByRole('dialog', { name: 'Add Plugin' });
const pluginEditor = await uiGetMonacoEditor(page, addPluginDialog);
await uiFillMonacoEditor(page, pluginEditor, '{"hide_credentials":
true}');
await addPluginDialog.getByRole('button', { name: 'Add' }).click();
await expect(addPluginDialog).toBeHidden();
// Verify the plugin was added
const pluginsSection = page.getByRole('group', { name: 'Plugins' });
await
expect(pluginsSection.getByTestId('plugin-key-auth')).toBeVisible();
// Submit the form
await routesPom.getAddBtn(page).click();
await uiHasToastMsg(page, {
hasText: 'Add Route Successfully',
});
});
```
##########
src/utils/producer.ts:
##########
@@ -50,6 +53,45 @@ export const produceRmDoubleUnderscoreKeys = produce((draft)
=> {
rmDoubleUnderscoreKeys(draft);
});
+// Preserve plugin entries with empty configs before cleaning
+// eslint-disable-next-line @typescript-eslint/no-explicit-any
+export const producePreservePlugins = produce((draft: any) => {
+ if (draft.plugins && typeof draft.plugins === 'object') {
+ // Mark plugin configs to preserve them from being cleaned
+ Object.keys(draft.plugins).forEach((pluginName) => {
+ const config = draft.plugins[pluginName];
+ if (
+ config &&
+ typeof config === 'object' &&
+ !Array.isArray(config) &&
+ Object.keys(config).length === 0
+ ) {
+ // Add a marker that will be removed later
+ draft.plugins[pluginName] = { __preserve: true };
+ }
+ });
+ }
Review Comment:
The new preserve-marker pipeline
(producePreservePlugins/produceRemovePreserveMarkers) appears redundant now
that produceDeepCleanEmptyKeys disables `emptyObjectsCleaner` (so `{}` should
already survive `fast-clean`). Keeping the marker logic adds complexity, uses
`any`, and introduces an internal `__preserve` field after
`produceRmDoubleUnderscoreKeys` has run (so it would be sent if the removal
step ever regresses). Consider removing the marker producers entirely and
relying on `emptyObjectsCleaner: false`, or (if you really want a targeted
approach) keep `emptyObjectsCleaner` enabled and expand preservation beyond
plugins to cover `discovery_args` too.
##########
src/utils/producer.ts:
##########
@@ -32,7 +32,10 @@ export const deepCleanEmptyKeys = <T extends object>(
export const produceDeepCleanEmptyKeys = (opts: ICleanerOptions = {}) =>
produce((draft) => {
- deepCleanEmptyKeys(draft, opts);
+ deepCleanEmptyKeys(draft, {
+ emptyObjectsCleaner: false,
+ ...opts,
+ });
Review Comment:
This change is intended to fix dropping empty objects in multiple places
(plugins and upstream `discovery_args`, per PR description/issues), but the
added e2e coverage only exercises the route/plugin path. Consider adding an e2e
(or request-level) test around editing an upstream with service discovery and
an empty `discovery_args` object to prevent regressions for Issue #3270 as well.
--
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]
