fix: check if label matchers have changed before composing (#1589)

wundergraph · Feb 12, 2025 · ead469c · ead469c
1 parent 473d4da
commit ead469c
Show file tree

Hide file tree

Showing 4 changed files with 264 additions and 3 deletions.
diff --git a/controlplane/src/core/bufservices/federated-graph/updateFederatedGraph.ts b/controlplane/src/core/bufservices/federated-graph/updateFederatedGraph.ts
@@ -162,7 +162,7 @@ export function updateFederatedGraph(
     });
 
     // Send webhook event only when we update label matchers because this causes schema update
-    if (req.labelMatchers.length > 0 || req.unsetLabelMatchers) {
+    if (result) {
       orgWebhooks.send(
         {
           eventName: OrganizationEventName.FEDERATED_GRAPH_SCHEMA_UPDATED,

diff --git a/controlplane/src/core/repositories/FederatedGraphRepository.ts b/controlplane/src/core/repositories/FederatedGraphRepository.ts
@@ -73,7 +73,7 @@ import {
 import { composeSubgraphsForContract, composeSubgraphsWithContracts } from '../composition/composition.js';
 import { SchemaDiff } from '../composition/schemaCheck.js';
 import { AdmissionError } from '../services/AdmissionWebhookController.js';
-import { normalizeLabelMatchers, normalizeLabels } from '../util.js';
+import { checkIfLabelMatchersChanged, normalizeLabelMatchers, normalizeLabels } from '../util.js';
 import { unsuccessfulBaseCompositionError } from '../errors/errors.js';
 import { ClickHouseClient } from '../clickhouse/index.js';
 import { ContractRepository } from './ContractRepository.js';
@@ -251,8 +251,15 @@ export class FederatedGraphRepository {
         await targetRepo.updateReadmeOfTarget({ id: data.targetId, readme: data.readme });
       }
 
+      const haveLabelMatchersChanged = checkIfLabelMatchersChanged({
+        isContract: !!federatedGraph.contract,
+        currentLabelMatchers: federatedGraph.labelMatchers,
+        newLabelMatchers: data.labelMatchers,
+        unsetLabelMatchers: data.unsetLabelMatchers,
+      });
+
       // Update label matchers (Is optional)
-      if (data.labelMatchers.length > 0 || data.unsetLabelMatchers) {
+      if (haveLabelMatchersChanged) {
         const labelMatchers = data.unsetLabelMatchers ? [] : normalizeLabelMatchers(data.labelMatchers);
 
         const contracts = await contractRepo.bySourceFederatedGraphId(federatedGraph.id);

diff --git a/controlplane/src/core/util.ts b/controlplane/src/core/util.ts
@@ -445,3 +445,42 @@ export function createBatches<T>(array: T[], batchSize: number): T[][] {
 
   return batches;
 }
+
+export const checkIfLabelMatchersChanged = (data: {
+  isContract: boolean;
+  currentLabelMatchers: string[];
+  newLabelMatchers: string[];
+  unsetLabelMatchers?: boolean;
+}) => {
+  if (data.isContract && data.newLabelMatchers.length === 0) {
+    return false;
+  }
+
+  // User tries to unset but no matchers exist, then nothing has changed
+  if (data.unsetLabelMatchers && data.currentLabelMatchers.length === 0) {
+    return false;
+  }
+
+  // If user tries to unset but matchers exist, then it has changed
+  if (data.unsetLabelMatchers) {
+    return true;
+  }
+
+  // Not a contract, not unsetting, no new matchers, then nothing has changed
+  if (data.newLabelMatchers.length === 0) {
+    return false;
+  }
+
+  // Not a contract, not unsetting but new matchers are passed, we need to check if they are different
+  if (data.newLabelMatchers.length !== data.currentLabelMatchers.length) {
+    return true;
+  }
+
+  for (const labelMatcher of data.newLabelMatchers) {
+    if (!data.currentLabelMatchers.includes(labelMatcher)) {
+      return true;
+    }
+  }
+
+  return false;
+};
diff --git a/controlplane/test/label.test.ts b/controlplane/test/label.test.ts
@@ -5,6 +5,7 @@ import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, test, vi
 import { ClickHouseClient } from '../src/core/clickhouse/index.js';
 import { afterAllSetup, beforeAllSetup, genID, genUniqueLabel } from '../src/core/test-util.js';
 import { Label } from '../src/types/index.js';
+import { checkIfLabelMatchersChanged } from '../src/core/util.js';
 import { createAndPublishSubgraph, createFederatedGraph, createThenPublishSubgraph, SetupTest } from './test-util.js';
 
 let dbname = '';
@@ -641,4 +642,218 @@ describe('Labels', (ctx) => {
 
     await server.close();
   });
+
+  test('Updating federated graph with same label matchers should not cause composition', async (testContext) => {
+    const { client, server } = await SetupTest({ dbname, chClient });
+
+    const fedGraph1Name = genID('fedGraph1');
+    const subgraph1Name = genID('subgraph1');
+    const label1 = genUniqueLabel('label1');
+    const label2 = genUniqueLabel('label2');
+
+    await createFederatedGraph(
+      client,
+      fedGraph1Name,
+      'default',
+      [`${joinLabel(label1)},${joinLabel(label2)}`],
+      'http://localhost:8081',
+    );
+
+    await createAndPublishSubgraph(
+      client,
+      subgraph1Name,
+      'default',
+      `type Query { name: String! }`,
+      [label1],
+      'http://localhost:8083',
+    );
+
+    const graph1 = await client.getCompositions({
+      fedGraphName: fedGraph1Name,
+      namespace: 'default',
+      startDate: formatISO(subDays(new Date(), 1)),
+      endDate: formatISO(addSeconds(new Date(), 5)),
+    });
+    expect(graph1.response?.code).toBe(EnumStatusCode.OK);
+    expect(graph1.compositions.length).toBe(1);
+
+    const res = await client.updateFederatedGraph({
+      name: fedGraph1Name,
+      namespace: 'default',
+      labelMatchers: [`${joinLabel(label1)},${joinLabel(label2)}`],
+      routingUrl: 'http://localhost:8089',
+    });
+    expect(res.response?.code).toBe(EnumStatusCode.OK);
+
+    const updatedGraph = await client.getCompositions({
+      fedGraphName: fedGraph1Name,
+      namespace: 'default',
+      startDate: formatISO(subDays(new Date(), 1)),
+      endDate: formatISO(addSeconds(new Date(), 5)),
+    });
+    expect(updatedGraph.response?.code).toBe(EnumStatusCode.OK);
+    expect(updatedGraph.compositions.length).toBe(1);
+
+    await server.close();
+  });
+
+  test('Unsetting label matchers for graph with no matchers to begin with should not cause compositions', async (testContext) => {
+    const { client, server } = await SetupTest({ dbname, chClient });
+
+    const fedGraph1Name = genID('fedGraph1');
+    const subgraph1Name = genID('subgraph1');
+
+    await createFederatedGraph(client, fedGraph1Name, 'default', [], 'http://localhost:8081');
+
+    await createAndPublishSubgraph(
+      client,
+      subgraph1Name,
+      'default',
+      `type Query { name: String! }`,
+      [],
+      'http://localhost:8083',
+    );
+
+    const graph1 = await client.getCompositions({
+      fedGraphName: fedGraph1Name,
+      namespace: 'default',
+      startDate: formatISO(subDays(new Date(), 1)),
+      endDate: formatISO(addSeconds(new Date(), 5)),
+    });
+    expect(graph1.response?.code).toBe(EnumStatusCode.OK);
+    expect(graph1.compositions.length).toBe(1);
+
+    const res = await client.updateFederatedGraph({
+      name: fedGraph1Name,
+      namespace: 'default',
+      unsetLabelMatchers: true,
+      routingUrl: 'http://localhost:8089',
+    });
+    expect(res.response?.code).toBe(EnumStatusCode.OK);
+
+    const updatedGraph = await client.getCompositions({
+      fedGraphName: fedGraph1Name,
+      namespace: 'default',
+      startDate: formatISO(subDays(new Date(), 1)),
+      endDate: formatISO(addSeconds(new Date(), 5)),
+    });
+    expect(updatedGraph.response?.code).toBe(EnumStatusCode.OK);
+    expect(updatedGraph.compositions.length).toBe(1);
+
+    await server.close();
+  });
+
+  test('Updating federated graph without any label matchers and also not unsetting should not cause compositions', async (testContext) => {
+    const { client, server } = await SetupTest({ dbname, chClient });
+
+    const fedGraph1Name = genID('fedGraph1');
+    const subgraph1Name = genID('subgraph1');
+    const label1 = genUniqueLabel('label1');
+    const label2 = genUniqueLabel('label2');
+
+    await createFederatedGraph(
+      client,
+      fedGraph1Name,
+      'default',
+      [`${joinLabel(label1)},${joinLabel(label2)}`],
+      'http://localhost:8081',
+    );
+
+    await createAndPublishSubgraph(
+      client,
+      subgraph1Name,
+      'default',
+      `type Query { name: String! }`,
+      [label1],
+      'http://localhost:8083',
+    );
+
+    const graph1 = await client.getCompositions({
+      fedGraphName: fedGraph1Name,
+      namespace: 'default',
+      startDate: formatISO(subDays(new Date(), 1)),
+      endDate: formatISO(addSeconds(new Date(), 5)),
+    });
+    expect(graph1.response?.code).toBe(EnumStatusCode.OK);
+    expect(graph1.compositions.length).toBe(1);
+
+    const res = await client.updateFederatedGraph({
+      name: fedGraph1Name,
+      namespace: 'default',
+      labelMatchers: [],
+      routingUrl: 'http://localhost:8089',
+    });
+    expect(res.response?.code).toBe(EnumStatusCode.OK);
+
+    const updatedGraph = await client.getCompositions({
+      fedGraphName: fedGraph1Name,
+      namespace: 'default',
+      startDate: formatISO(subDays(new Date(), 1)),
+      endDate: formatISO(addSeconds(new Date(), 5)),
+    });
+    expect(updatedGraph.response?.code).toBe(EnumStatusCode.OK);
+    expect(updatedGraph.compositions.length).toBe(1);
+
+    await server.close();
+  });
+
+  test('Check if label matchers changed', () => {
+    // Case 1: isContract is true and newLabelMatchers is empty
+    let result = checkIfLabelMatchersChanged({
+      isContract: true,
+      currentLabelMatchers: [],
+      newLabelMatchers: [],
+    });
+    expect(result).toBe(false);
+
+    // Case 2: unsetLabelMatchers is true and currentLabelMatchers is empty
+    result = checkIfLabelMatchersChanged({
+      isContract: false,
+      currentLabelMatchers: [],
+      newLabelMatchers: [],
+      unsetLabelMatchers: true,
+    });
+    expect(result).toBe(false);
+
+    // Case 3: unsetLabelMatchers is true and currentLabelMatchers is not empty
+    result = checkIfLabelMatchersChanged({
+      isContract: false,
+      currentLabelMatchers: ['label1'],
+      newLabelMatchers: [],
+      unsetLabelMatchers: true,
+    });
+    expect(result).toBe(true);
+
+    // Case 4: newLabelMatchers is empty and we are not unsetting
+    result = checkIfLabelMatchersChanged({
+      isContract: false,
+      currentLabelMatchers: ['label1'],
+      newLabelMatchers: [],
+    });
+    expect(result).toBe(false);
+
+    // Case 5: newLabelMatchers length is different from currentLabelMatchers length
+    result = checkIfLabelMatchersChanged({
+      isContract: false,
+      currentLabelMatchers: ['label1'],
+      newLabelMatchers: ['label1', 'label2'],
+    });
+    expect(result).toBe(true);
+
+    // Case 6: newLabelMatchers contains different labels from currentLabelMatchers
+    result = checkIfLabelMatchersChanged({
+      isContract: false,
+      currentLabelMatchers: ['label1'],
+      newLabelMatchers: ['label2'],
+    });
+    expect(result).toBe(true);
+
+    // Case 7: newLabelMatchers is the same as currentLabelMatchers
+    result = checkIfLabelMatchersChanged({
+      isContract: false,
+      currentLabelMatchers: ['label1'],
+      newLabelMatchers: ['label1'],
+    });
+    expect(result).toBe(false);
+  });
 });