refactor: [M3-7920, M3-7930] – Refactor VPC queries #10322

dwiley-akamai · 2024-03-27T17:51:16Z

Description 📝

Refactor the VPC queries to use createQueryKeys from query-key-factory.

The main objective was to achieve parity with existing functionality, although some small enhancements at the margins were done as well.

Changes 🔄

Use createQueryKeys to generate VPC & subnet query keys
Update mock region data to include VPC capability
Add new useAllVPCsQuery and use it in VPCPanel.tsx & useVPCConfigInterface.tsx
Remove manual uses of vpcQueryKey and subnetQueryKey throughout codebase, notably for various query invalidations
Remove unused useSubnetQuery

Target release date 🗓️

4/15/24

How to test 🧪

Verification steps

Essentially the entire VPC offering in Cloud needs to be tested to ensure nothing has broken with these changes.

VPC landing page
- query for paginated results should refresh upon VPC creation
Assign/Unassign Linode flow via drawer accessed from Subnets table
- should invalidate the paginated VPC, individual VPC, and VPC subnets queries
Assigning Linode to VPC via Linode Config dialog
- all VPC queries should be invalidated
Linode Create flow
- a newly-created linode assigned to a VPC should have the VPC section present in the Linode Detail header
Linode Detail page
- the VPC section in the Linode Detail header should be accurate, and the "IP Addresses" table on the "Network" tab should contain the expected VPC-related IPs
Deleting a Linode
- the 'all' VPCs, paginated VPCs, individual VPC, and VPC subnets queries should be invalidated upon Linode deletion

As an Author I have considered 🤔

…ConfigInterface.ts; include 'VPCs' as capability for mock regions

…sts.ts + update imports across multiple files

dwiley-akamai · 2024-04-01T20:41:39Z

packages/manager/src/queries/vpcs/vpcs.ts

+    contextQueries: {
+      subnet: (subnetId: number) => ({
+        queryFn: () => getSubnet(vpcId, subnetId),
+        queryKey: null,
+      }),
+      subnets: {
+        paginated: (params: Params = {}, filter: Filter = {}) => ({
+          queryFn: () => getSubnets(vpcId, params, filter),
+          queryKey: [params, filter],
+        }),
+      },
+    },
+    queryFn: () => getVPC(vpcId),
+    queryKey: [vpcId],


@bnussman-akamai Does this look correct to you? For some reason, when I go into useSubnetsQuery and begin refactoring it, I type ...vpcQueries.vpc(vpcID). but _ctx doesn't get picked up on as something that exists on the object.

I pulled an example from the docs,

export const users = createQueryKeys('users', { detail: (userId: string) => ({ contextQueries: { likes: { queryFn: () => api.getUserLikes(userId), queryKey: null, }, }, queryFn: () => api.getUser(userId), queryKey: [userId], }), });

just to test the autocomplete and see if there's a syntax issue, but it successfully picked up on users.detail(userId)._ctx as expected.

Am I overlooking something here?

Give this a try

export const vpcQueries = createQueryKeys('vpcs', { vpc: (vpcId: number) => ({ contextQueries: { subnets: { contextQueries: { paginated: (params: Params = {}, filter: Filter = {}) => ({ queryFn: () => getSubnets(vpcId, params, filter), queryKey: [params, filter], }), subnet: (subnetId: number) => ({ queryFn: () => getSubnet(vpcId, subnetId), queryKey: [subnetId], }), }, queryKey: null, }, }, queryFn: () => getVPC(vpcId), queryKey: [vpcId], }), vpcs: { contextQueries: { all: { queryFn: getAllVPCsRequest, queryKey: null, }, paginated: (params: Params = {}, filter: Filter = {}) => ({ queryFn: () => getVPCs(params, filter), queryKey: [params, filter], }), }, queryKey: null, }, });

I added queryKey: [subnetId] to the subnet definition

I added more nesting to the subnets queries

… should invalidate all VPC queries still)

…since LinodeEntityDetail.tsx pulls in from that one

…dual VPC, and VPC subnets queries should be invalidated upon Linode deletion

…tUnassignLinodesDrawer.tsx, and SubnetAssignLinodesDrawer.tsx (assigning and unassigning Linodes via the drawer should invalidate the paginated VPC, individual VPC, and VPC subnets queries)

dwiley-akamai · 2024-04-03T22:17:56Z

packages/manager/src/queries/vpcs/vpcs.ts

This query previously existed:

export const useSubnetQuery = (vpcID: number, subnetID: number) => { return useQuery<Subnet, APIError[]>( [vpcQueryKey, 'vpc', vpcID, subnetQueryKey, 'subnet', subnetID], () => getSubnet(vpcID, subnetID) ); };

Most of the original queries were written before actual dev work on the UI began last summer, and it turns out this was never used (there's nothing approximating a Subnet Details page) so I removed it.

dwiley-akamai · 2024-04-03T22:24:33Z

packages/manager/src/hooks/useUnassignLinode.ts


 import type {
  APIError,
  DeleteLinodeConfigInterfacePayload,
 } from '@linode/api-v4';

-type IdsForUnassignLinode = DeleteLinodeConfigInterfacePayload & {
-  subnetId: number;
+interface IdsForUnassignLinode extends DeleteLinodeConfigInterfacePayload {


match the new convention (see #10309 for more)

dwiley-akamai · 2024-04-03T22:26:04Z

packages/manager/src/hooks/useUnassignLinode.ts


 import type {
  APIError,
  DeleteLinodeConfigInterfacePayload,
 } from '@linode/api-v4';

-type IdsForUnassignLinode = DeleteLinodeConfigInterfacePayload & {
-  subnetId: number;


Nothing at the level of [vpcQueryKey, 'vpc', vpcId, subnetQueryKey, 'subnet', subnetId] was ever used in the actual code so this was superfluous

dwiley-akamai · 2024-04-03T22:32:39Z

packages/manager/src/queries/vpcs/vpcs.ts

+    queryFn: () => getVPC(vpcId),
+    queryKey: [vpcId],
+  }),
+  vpcs: {


This results in "vpcs","vpcs" [...] which I do not like given the redundancy, but I think we have to accept?

Could we "un-nest" the all and paginated queries?

Like this:

export const vpcQueries = createQueryKeys('vpcs', { all: { queryFn: getAllVPCsRequest, queryKey: null, }, paginated: (params: Params = {}, filter: Filter = {}) => ({ queryFn: () => getVPCs(params, filter), queryKey: [params, filter], }), vpc: (vpcId: number) => ({ contextQueries: { subnets: { contextQueries: { paginated: (params: Params = {}, filter: Filter = {}) => ({ queryFn: () => getSubnets(vpcId, params, filter), queryKey: [params, filter], }), subnet: (subnetId: number) => ({ queryFn: () => getSubnet(vpcId, subnetId), queryKey: [subnetId], }), }, queryKey: null, }, }, queryFn: () => getVPC(vpcId), queryKey: [vpcId], }), })

@bnussman-akamai Yes, that works and allows us to clean things up a bit 👍🏾 Going to push that change

…ng need for contextQueries/_ctx and to clean up the end result of the query key

github-actions · 2024-04-04T22:41:55Z

Coverage Report: ✅
Base Coverage: 81.79%
Current Coverage: 81.8%

packages/manager/src/features/Linodes/LinodesCreate/LinodeCreateContainer.tsx

packages/manager/src/features/Linodes/LinodesDetail/LinodeConfigs/LinodeConfigDialog.tsx

bnussman-akamai · 2024-04-08T13:56:22Z

packages/manager/src/hooks/useVPCConfigInterface.ts

Can we refactor this hook to use useVPCQuery instead of useAllVPCsQuery? This should be a solid optimization over fetching every single VPC

We previously used useVPCsQuery here, but I think that would pose an issue for some data on the Linode Detail page (see LinodeEntityDetail.tsx and LinodeIPAddress.tsx) for customers with lots of VPCs if the VPC wasn't on the first page of results?

I'm proposing we just fetch the singular VPC. I agree useVPCsQuery isn't a reliable way to do this

Whoops, I misread your original comment. Yes, that worked fine from my testing so I pushed the refactor in the most recent commit.

packages/manager/src/queries/vpcs/vpcs.ts

bnussman-akamai

Looking great, just a few more things

packages/manager/src/hooks/useUnassignLinode.ts

bnussman-akamai · 2024-04-08T20:13:40Z

packages/manager/src/hooks/useVPCConfigInterface.ts

@@ -35,6 +22,10 @@ export const useVPCConfigInterface = (linodeId: number) => {
    return interfaceWithVPC;
  });

+  const { data: vpcLinodeIsAssignedTo } = useVPCQuery(
+    configInterfaceWithVPC?.vpc_id ?? -1


We might want to disable this query so we don't fetch when there is no VPC interface on a Linode.

Suggested change

configInterfaceWithVPC?.vpc_id ?? -1

configInterfaceWithVPC?.vpc_id ?? -1,

Boolean(configInterfaceWithVPC)

dwiley-akamai · 2024-04-10T17:54:08Z

@cliu-akamai There's an E2E failure for linode-config.spec.ts (failed on the previous run and fails locally) that I haven't been able to diagnose -- could you take a look at that?

cliu-akamai · 2024-04-10T23:29:16Z

@cliu-akamai There's an E2E failure for linode-config.spec.ts (failed on the previous run and fails locally) that I haven't been able to diagnose -- could you take a look at that?

Sure thing!

…/vpcs/vpcs.ts path

cliu-akamai · 2024-04-11T17:32:20Z

If we stop querying the VPCs when accessing endpoint /linodes/${linodeId}/configurations, we should remove Lines 149 and 304 in linode-config.spec.ts to avoid test failure.

bnussman-akamai · 2024-04-11T18:23:49Z

Looks like deleting lines 138, 149, 292, and 303 fix the failures. I think this makes sense because of the optimization we made in useVPCConfigInterface. We no longer need to GET /v4/vpcs because we now just fetch the singular VPC using GET /v4/vpcs/:id

I'm happy to push that change if you want me to!

dwiley-akamai · 2024-04-11T19:14:25Z

Looks like deleting lines 138, 149, 292, and 303 fix the failures. I think this makes sense because of the optimization we made in useVPCConfigInterface. We no longer need to GET /v4/vpcs because we now just fetch the singular VPC using GET /v4/vpcs/:id

Initially it was a bit confusing because queryClient.invalidateQueries(vpcQueries.paginated._def) in LinodeConfigDialog.tsx seemed to correspond with the @getVPCs in the test, but in terms of the actual sequence of interactions it makes sense (that invalidation doesn't happen until the user clicks "Save Changes" and it goes through successfully). So I think we're good on that aspect 👍🏾 cc @cliu-akamai

Another point is looking into removing the mockAppendFeatureFlags logic in linode-config.spec.ts since VPC isn't feature flagged anymore. That's not in-scope for this ticket, but commenting those pieces out does make the test fail again locally

bnussman-akamai

Looks great 🔥 Thanks for doing this!

packages/manager/src/hooks/useUnassignLinode.ts

packages/manager/src/features/Linodes/LinodesCreate/LinodeCreateContainer.tsx

cpathipa

Nice work @dwiley-akamai!
confirming on the VPC flow and no regressions found.

dwiley-akamai · 2024-04-12T19:49:24Z

@bnussman-akamai Good catches, those were appropriate invalidations to include. Added in the latest commit

DevDW added 2 commits March 25, 2024 15:43

add new useAllVPCsQuery() query and use it in VPCPanel.tsx and useVPC…

5a02b99

…ConfigInterface.ts; include 'VPCs' as capability for mock regions

Add VPC query key factory, split requests into src/queries/vpcs/reque…

f12a505

…sts.ts + update imports across multiple files

dwiley-akamai added React Query Relating to the transition to use React Query VPC Relating to VPC project labels Mar 27, 2024

dwiley-akamai self-assigned this Mar 27, 2024

dwiley-akamai added the Work in Progress label Mar 27, 2024

Attempt to flesh out vpcQueries for subnet(s)

6f8c994

dwiley-akamai commented Apr 1, 2024

View reviewed changes

DevDW added 6 commits April 2, 2024 16:12

Refactor and clean up subnet queries

c5ca05a

Update LinodeConfigDialog.tsx (assigning VPC via Linode Config dialog…

fa03346

… should invalidate all VPC queries still)

Update LinodeCreaeContainter.tsx and invalidate the 'all' VPCs query …

7ce7435

…since LinodeEntityDetail.tsx pulls in from that one

Update DeleteLinodeDialog.tsx (the 'all' VPCs, paginated VPCs, indivi…

b601e0a

…dual VPC, and VPC subnets queries should be invalidated upon Linode deletion

Update useUnassignLinode() and cleanup in useUnassignLinode.ts, Subne…

407c9b1

…tUnassignLinodesDrawer.tsx, and SubnetAssignLinodesDrawer.tsx (assigning and unassigning Linodes via the drawer should invalidate the paginated VPC, individual VPC, and VPC subnets queries)

Added changeset: Refactor and streamline VPC queries

9281019

dwiley-akamai commented Apr 3, 2024

View reviewed changes

dwiley-akamai removed the Work in Progress label Apr 3, 2024

DevDW added 2 commits April 3, 2024 18:53

Merge in latest develop and resolve conflict

1a3ef4f

The end of the road has come for vpcQueryKey and subnetQueryKey

ae75635

dwiley-akamai marked this pull request as ready for review April 3, 2024 22:56

dwiley-akamai requested a review from a team as a code owner April 3, 2024 22:56

dwiley-akamai requested review from jdamore-linode, cpathipa and bnussman-akamai and removed request for a team April 3, 2024 22:56

De-nest the 'all' and 'paginated' query objects to simplify by removi…

1581b3f

…ng need for contextQueries/_ctx and to clean up the end result of the query key

bnussman-akamai reviewed Apr 8, 2024

View reviewed changes

DevDW added 2 commits April 8, 2024 13:50

Feedback: more explicit invalidations

25324b8

Refactor useVPCConfigInterface()

eab0962

bnussman-akamai reviewed Apr 8, 2024

View reviewed changes

Feedback: couple of tweaks

cf9f3d3

bnussman-akamai mentioned this pull request Apr 8, 2024

upcoming: [M3-7924] - Linode Create Refactor - VPC - Part 10 #10354

Merged

12 tasks

update LinodeEntityDetail.test.tsx

be3d67e

dwiley-akamai requested review from cliu-akamai and removed request for cliu-akamai and jdamore-linode April 9, 2024 15:09

DevDW added 2 commits April 11, 2024 11:57

Merge in latest develop and resolve conflict

e83a052

delete old queries/vpcs.ts file and update imports to use new queries…

e3eb837

…/vpcs/vpcs.ts path

Update linode-config.spec.ts to get VPC-related assertions passing

f1d9a56

dwiley-akamai requested a review from a team as a code owner April 11, 2024 19:05

dwiley-akamai requested review from cliu-akamai and removed request for a team April 11, 2024 19:05

bnussman-akamai approved these changes Apr 12, 2024

View reviewed changes

packages/manager/src/hooks/useUnassignLinode.ts Show resolved Hide resolved

packages/manager/src/features/Linodes/LinodesCreate/LinodeCreateContainer.tsx Show resolved Hide resolved

cpathipa approved these changes Apr 12, 2024

View reviewed changes

Feedback: a few other query invalidations

9fc2ac4

dwiley-akamai merged commit 6bffda4 into linode:develop Apr 15, 2024
18 checks passed

dwiley-akamai deleted the M3-7920-7930-vpc-queries-updates branch April 15, 2024 20:51

mjac0bs pushed a commit to mjac0bs/manager that referenced this pull request Apr 18, 2024

refactor: [M3-7920, M3-7930] – Refactor VPC queries (linode#10322)

9f4fe2b

This was referenced Apr 25, 2024

Release v.118.0 - release → staging #10409

Closed

Release v1.118.0 - release → staging #10410

Merged

Release v1.118.0 - staging → master #10416

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: [M3-7920, M3-7930] – Refactor VPC queries #10322

refactor: [M3-7920, M3-7930] – Refactor VPC queries #10322

dwiley-akamai commented Mar 27, 2024 •

edited

dwiley-akamai Apr 1, 2024

bnussman-akamai Apr 1, 2024

dwiley-akamai Apr 3, 2024

dwiley-akamai Apr 3, 2024

dwiley-akamai Apr 3, 2024

dwiley-akamai Apr 3, 2024

bnussman-akamai Apr 4, 2024

dwiley-akamai Apr 4, 2024

github-actions bot commented Apr 4, 2024 •

edited

bnussman-akamai Apr 8, 2024

dwiley-akamai Apr 8, 2024

bnussman-akamai Apr 8, 2024

dwiley-akamai Apr 8, 2024

bnussman-akamai left a comment

bnussman-akamai Apr 8, 2024

dwiley-akamai commented Apr 10, 2024

cliu-akamai commented Apr 10, 2024

cliu-akamai commented Apr 11, 2024

bnussman-akamai commented Apr 11, 2024

dwiley-akamai commented Apr 11, 2024

bnussman-akamai left a comment

cpathipa left a comment

dwiley-akamai commented Apr 12, 2024

	configInterfaceWithVPC?.vpc_id ?? -1
	configInterfaceWithVPC?.vpc_id ?? -1,
	Boolean(configInterfaceWithVPC)

refactor: [M3-7920, M3-7930] – Refactor VPC queries #10322

refactor: [M3-7920, M3-7930] – Refactor VPC queries #10322

Conversation

dwiley-akamai commented Mar 27, 2024 • edited

Description 📝

Changes 🔄

Target release date 🗓️

How to test 🧪

Verification steps

As an Author I have considered 🤔

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 4, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnussman-akamai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwiley-akamai commented Apr 10, 2024

cliu-akamai commented Apr 10, 2024

cliu-akamai commented Apr 11, 2024

bnussman-akamai commented Apr 11, 2024

dwiley-akamai commented Apr 11, 2024

bnussman-akamai left a comment

Choose a reason for hiding this comment

cpathipa left a comment

Choose a reason for hiding this comment

dwiley-akamai commented Apr 12, 2024

dwiley-akamai commented Mar 27, 2024 •

edited

github-actions bot commented Apr 4, 2024 •

edited