Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suspensify PermissionsProvider using useWrappedQuery #21440

Closed
wants to merge 8 commits into from
Closed

Suspensify PermissionsProvider using useWrappedQuery #21440

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a0a2510
suspensify permissions providers
salazarm Apr 26, 2024
0a2fcf8
add suspense boundaries
salazarm Apr 26, 2024
ce04373
default permissions
salazarm Apr 26, 2024
7023b6f
default permissions
salazarm Apr 26, 2024
cb606c0
update test
salazarm Apr 26, 2024
f88888f
.
salazarm Apr 26, 2024
46c5c5d
more tests??
salazarm Apr 26, 2024
f43e10c
better test
salazarm Apr 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 20 additions & 18 deletions js_modules/dagster-ui/packages/app-oss/src/App.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,23 +41,25 @@ const appCache = createAppCache();
// eslint-disable-next-line import/no-default-export
export default function AppPage() {
return (
<RecoilRoot>
<InjectedComponents>
<LiveDataPollRateContext.Provider value={liveDataPollRate ?? 2000}>
<AppProvider appCache={appCache} config={config}>
<AppTopNav allowGlobalReload>
<HelpMenu showContactSales={false} />
<UserSettingsButton />
</AppTopNav>
<App>
<ContentRoot />
<Suspense>
<CommunityNux />
</Suspense>
</App>
</AppProvider>
</LiveDataPollRateContext.Provider>
</InjectedComponents>
</RecoilRoot>
<Suspense fallback={<div />}>
<RecoilRoot>
<InjectedComponents>
<LiveDataPollRateContext.Provider value={liveDataPollRate ?? 2000}>
<AppProvider appCache={appCache} config={config}>
<AppTopNav allowGlobalReload>
<HelpMenu showContactSales={false} />
<UserSettingsButton />
</AppTopNav>
<App>
<ContentRoot />
<Suspense>
<CommunityNux />
</Suspense>
</App>
</AppProvider>
</LiveDataPollRateContext.Provider>
</InjectedComponents>
</RecoilRoot>
</Suspense>
);
}
75 changes: 40 additions & 35 deletions js_modules/dagster-ui/packages/ui-core/src/app/ContentRoot.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import {ErrorBoundary, MainContent} from '@dagster-io/ui-components';
import {Suspense, lazy, memo, useEffect, useRef} from 'react';
import {lazy, memo, useEffect, useRef} from 'react';
import {Route, Switch, useLocation} from 'react-router-dom';

import {TrackedSuspense} from './TrackedSuspense';
import {AssetFeatureProvider} from '../assets/AssetFeatureContext';
import {AssetsOverview} from '../assets/AssetsOverview';

Expand Down Expand Up @@ -35,97 +36,101 @@ export const ContentRoot = memo(() => {
<ErrorBoundary region="page" resetErrorOnChange={[pathname]}>
<Switch>
<Route path="/asset-groups(/?.*)">
<Suspense fallback={<div />}>
<TrackedSuspense fallback={<div />} id={id('asset-groups')}>
<AssetsGroupsGlobalGraphRoot />
</Suspense>
</TrackedSuspense>
</Route>
<Route path="/assets(/?.*)">
<Suspense fallback={<div />}>
<TrackedSuspense fallback={<div />} id={id('assets')}>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have two questions here:

  1. Why is it necessary to put the Suspense inside the Route and not inside the Switch on line 37? Does this really need to be repeated for every route separately like this? Is this primarily so the id can be passed?

  2. If the reason for Separate Pandas Dataframe Solid into Two Sources #1 is so that the suspense instance is replaced when you navigate, do these /* type routes all share the same tracked suspense? Is that ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not necessary, I was just replacing the existing Suspense calls with TrackedSuspense in a follow up we can consolidate these but didn't want to unknowingly break anything by doing that

<AssetFeatureProvider>
<AssetsOverview
headerBreadcrumbs={[{text: 'Assets', href: '/assets'}]}
documentTitlePrefix="Assets"
/>
</AssetFeatureProvider>
</Suspense>
</TrackedSuspense>
</Route>
<Route path="/runs" exact>
<Suspense fallback={<div />}>
<TrackedSuspense fallback={<div />} id={id('runs')}>
<RunsRoot />
</Suspense>
</TrackedSuspense>
</Route>
<Route path="/runs/scheduled" exact>
<Suspense fallback={<div />}>
<TrackedSuspense fallback={<div />} id={id('runs/scheduled')}>
<ScheduledRunListRoot />
</Suspense>
</TrackedSuspense>
</Route>
<Route path="/runs/:runId" exact>
<Suspense fallback={<div />}>
<TrackedSuspense fallback={<div />} id={id('runs/:runId')}>
<RunRoot />
</Suspense>
</TrackedSuspense>
</Route>
<Route path="/snapshots/:pipelinePath/:tab?">
<Suspense fallback={<div />}>
<TrackedSuspense fallback={<div />} id={id('snapshots')}>
<SnapshotRoot />
</Suspense>
</TrackedSuspense>
</Route>
<Route path="/health">
<Suspense fallback={<div />}>
<TrackedSuspense fallback={<div />} id={id('health')}>
<InstanceHealthPage />
</Suspense>
</TrackedSuspense>
</Route>
<Route path="/concurrency">
<Suspense fallback={<div />}>
<TrackedSuspense fallback={<div />} id={id('concurrency')}>
<InstanceConcurrencyPage />
</Suspense>
</TrackedSuspense>
</Route>
<Route path="/config">
<Suspense fallback={<div />}>
<TrackedSuspense fallback={<div />} id={id('config')}>
<InstanceConfig />
</Suspense>
</TrackedSuspense>
</Route>
<Route path="/locations" exact>
<Suspense fallback={<div />}>
<TrackedSuspense fallback={<div />} id={id('"locations"')}>
<CodeLocationsPage />
</Suspense>
</TrackedSuspense>
</Route>
<Route path="/locations">
<Suspense fallback={<div />}>
<TrackedSuspense fallback={<div />} id={id('locations')}>
<WorkspaceRoot />
</Suspense>
</TrackedSuspense>
</Route>
<Route path="/guess/:jobPath">
<Suspense fallback={<div />}>
<TrackedSuspense fallback={<div />} id={id('guess/:jobPath')}>
<GuessJobLocationRoot />
</Suspense>
</TrackedSuspense>
</Route>
<Route path="/overview">
<Suspense fallback={<div />}>
<TrackedSuspense id={id('Overview')} fallback={<div />}>
<OverviewRoot />
</Suspense>
</TrackedSuspense>
</Route>
<Route path="/jobs">
<Suspense fallback={<div />}>
<TrackedSuspense id={id('JobsRoot')} fallback={<div />}>
<JobsRoot />
</Suspense>
</TrackedSuspense>
</Route>
<Route path="/automation">
<Suspense fallback={<div />}>
<TrackedSuspense fallback={<div />} id={id('automation')}>
<AutomationRoot />
</Suspense>
</TrackedSuspense>
</Route>
<Route path="/settings">
<Suspense fallback={<div />}>
<TrackedSuspense fallback={<div />} id={id('settings')}>
<SettingsRoot />
</Suspense>
</TrackedSuspense>
</Route>
<Route path="*">
<Suspense fallback={<div />}>
<TrackedSuspense fallback={<div />} id={id('*')}>
<FallthroughRoot />
</Suspense>
</TrackedSuspense>
</Route>
</Switch>
</ErrorBoundary>
</MainContent>
);
});

function id(str: string) {
return `ContentRoot:${str}`;
}
92 changes: 54 additions & 38 deletions js_modules/dagster-ui/packages/ui-core/src/app/Permissions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
import {gql, useQuery} from '@apollo/client';
import {ApolloError, gql} from '@apollo/client';
import * as React from 'react';

import {
PermissionFragment,
PermissionsQuery,
PermissionsQueryVariables,
} from './types/Permissions.types';
import {useWrappedQuery} from '../hooks/useWrappedQuery';
import {wrapPromise} from '../utils/wrapPromise';

// used in tests, to ensure against permission renames. Should make sure that the mapping in
// extractPermissions is handled correctly
Expand Down Expand Up @@ -111,53 +113,67 @@ type PermissionDisabledReasons = Record<keyof PermissionsMap, string>;
export type PermissionsState = {
permissions: PermissionBooleans;
disabledReasons: PermissionDisabledReasons;
loading: boolean;
};

type PermissionsContextType = {
export type PermissionsResult = {
error?: ApolloError;
unscopedPermissions: PermissionsMap;
locationPermissions: Record<string, PermissionsMap>;
loading: boolean;
// Raw unscoped permission data, for Cloud extraction
rawUnscopedData: PermissionFragment[];
};

type PermissionsContextType = ReturnType<typeof wrapPromise<PermissionsResult>>;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice for your wrapPromise util to expose a type helper that makes this Wrapped<PermissionsResult>, I expect we'll need this in a lot of places


export const PermissionsContext = React.createContext<PermissionsContextType>({
unscopedPermissions: extractPermissions([]),
locationPermissions: {},
loading: true,
rawUnscopedData: [],
read() {
return {
error: undefined,
unscopedPermissions: extractPermissions([]),
rawUnscopedData: [],
locationPermissions: {},
};
},
});

export const PermissionsProvider = (props: {children: React.ReactNode}) => {
const {data, loading} = useQuery<PermissionsQuery, PermissionsQueryVariables>(PERMISSIONS_QUERY, {
fetchPolicy: 'cache-first', // Not expected to change after initial load.
});
const wrappedQuery = useWrappedQuery<
PermissionsQuery,
PermissionsQueryVariables,
PermissionsResult
>(
{
query: PERMISSIONS_QUERY,
fetchPolicy: 'cache-first', // Not expected to change after initial load.
},
async (result) => {
const {data, error} = result;

const value = React.useMemo(() => {
const unscopedPermissionsRaw = data?.unscopedPermissions || [];
const unscopedPermissions = extractPermissions(unscopedPermissionsRaw);
const unscopedPermissionsRaw = data?.unscopedPermissions || [];
const unscopedPermissions = extractPermissions(unscopedPermissionsRaw);

const locationEntries =
data?.workspaceOrError.__typename === 'Workspace'
? data.workspaceOrError.locationEntries
: [];
const locationEntries =
data?.workspaceOrError.__typename === 'Workspace'
? data.workspaceOrError.locationEntries
: [];

const locationPermissions: Record<string, PermissionsMap> = {};
locationEntries.forEach((locationEntry) => {
const {name, permissions} = locationEntry;
locationPermissions[name] = extractPermissions(permissions, unscopedPermissionsRaw);
});
const locationPermissions: Record<string, PermissionsMap> = {};
locationEntries.forEach((locationEntry) => {
const {name, permissions} = locationEntry;
locationPermissions[name] = extractPermissions(permissions, unscopedPermissionsRaw);
});

return {
unscopedPermissions,
locationPermissions,
loading,
rawUnscopedData: unscopedPermissionsRaw,
};
}, [data, loading]);
return {
error,
unscopedPermissions,
locationPermissions,
rawUnscopedData: unscopedPermissionsRaw,
};
Comment on lines +148 to +170
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks good to me! Just from reading this it's not clear that the use of useWrapped has suspense implications, it sorta just looks like a stylistic change moving the data transform into the hook. Maybe we call out in the naming that this significantly changes the loading semantics.

},
);

return <PermissionsContext.Provider value={value}>{props.children}</PermissionsContext.Provider>;
return (
<PermissionsContext.Provider value={wrappedQuery}>{props.children}</PermissionsContext.Provider>
);
};

export const permissionResultForKey = (
Expand Down Expand Up @@ -191,7 +207,8 @@ const unpackPermissions = (
* Retrieve a permission that is intentionally unscoped.
*/
export const useUnscopedPermissions = (): PermissionsState => {
const {unscopedPermissions, loading} = React.useContext(PermissionsContext);
const {read} = React.useContext(PermissionsContext);
const {unscopedPermissions} = read();
const unpacked = React.useMemo(
() => unpackPermissions(unscopedPermissions),
[unscopedPermissions],
Expand All @@ -201,9 +218,8 @@ export const useUnscopedPermissions = (): PermissionsState => {
return {
permissions: unpacked.booleans,
disabledReasons: unpacked.disabledReasons,
loading,
};
}, [unpacked, loading]);
}, [unpacked]);
};

/**
Expand All @@ -214,7 +230,8 @@ export const useUnscopedPermissions = (): PermissionsState => {
export const usePermissionsForLocation = (
locationName: string | null | undefined,
): PermissionsState => {
const {unscopedPermissions, locationPermissions, loading} = React.useContext(PermissionsContext);
const {read} = React.useContext(PermissionsContext);
const {unscopedPermissions, locationPermissions} = read();
let permissionsForLocation = unscopedPermissions;
if (locationName && locationPermissions.hasOwnProperty(locationName)) {
permissionsForLocation = locationPermissions[locationName]!;
Expand All @@ -225,9 +242,8 @@ export const usePermissionsForLocation = (
return {
permissions: unpacked.booleans,
disabledReasons: unpacked.disabledReasons,
loading,
};
}, [unpacked, loading]);
}, [unpacked]);
};

export const PERMISSIONS_QUERY = gql`
Expand Down
3 changes: 1 addition & 2 deletions js_modules/dagster-ui/packages/ui-core/src/app/__tests__/CustomConfirmationProvider.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
import {Button} from '@dagster-io/ui-components';
import {render, screen, waitFor} from '@testing-library/react';
import {act, render, screen, waitFor} from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import {useState} from 'react';
import {act} from 'react-dom/test-utils';

import {CustomConfirmationProvider, useConfirmation} from '../CustomConfirmationProvider';

Expand Down
Loading