Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

remove deprecated ConfigurationCascade GraphQL field #63935

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

remove deprecated ConfigurationCascade GraphQL field #63935

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 8 additions & 8 deletions client/browser/src/integration/github.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,8 @@ describe('GitHub', () => {
})

testContext.overrideGraphQL({
ViewerConfiguration: () => ({
viewerConfiguration: {
ViewerSettings: () => ({
viewerSettings: {
subjects: [],
merged: { contents: '', messages: [] },
},
Expand Down Expand Up @@ -158,8 +158,8 @@ describe('GitHub', () => {
// extensions: extensionSettings,
// }
// testContext.overrideGraphQL({
// ViewerConfiguration: () => ({
// viewerConfiguration: {
// ViewerSettings: () => ({
// viewerSettings: {
// subjects: [
// {
// __typename: 'User',
Expand Down Expand Up @@ -321,8 +321,8 @@ describe('GitHub', () => {
extensions: extensionSettings,
}
testContext.overrideGraphQL({
ViewerConfiguration: () => ({
viewerConfiguration: {
ViewerSettings: () => ({
viewerSettings: {
subjects: [
{
__typename: 'User',
Expand Down Expand Up @@ -643,8 +643,8 @@ describe('GitHub', () => {
extensions: extensionSettings,
}
testContext.overrideGraphQL({
ViewerConfiguration: () => ({
viewerConfiguration: {
ViewerSettings: () => ({
viewerSettings: {
subjects: [
{
__typename: 'User',
Expand Down
8 changes: 4 additions & 4 deletions client/browser/src/integration/gitlab.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,8 @@ describe('GitLab', () => {
})

testContext.overrideGraphQL({
ViewerConfiguration: () => ({
viewerConfiguration: {
ViewerSettings: () => ({
viewerSettings: {
subjects: [],
merged: { contents: '', messages: [] },
},
Expand Down Expand Up @@ -152,8 +152,8 @@ describe('GitLab', () => {
extensions: extensionSettings,
}
testContext.overrideGraphQL({
ViewerConfiguration: () => ({
viewerConfiguration: {
ViewerSettings: () => ({
viewerSettings: {
subjects: [
{
__typename: 'User',
Expand Down
34 changes: 16 additions & 18 deletions client/browser/src/shared/platform/settings.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {
} from '@sourcegraph/shared/src/settings/settings'

import { observeStorageKey, storage } from '../../browser-extension/web-extension-api/storage'
import type { ViewerConfigurationResult } from '../../graphql-operations'
import type { ViewerSettingsResult } from '../../graphql-operations'
import { isInPage } from '../context'

const inPageClientSettingsKey = 'sourcegraphClientSettings'
Expand All @@ -35,7 +35,8 @@ function observeLocalStorageKey(key: string, defaultValue: string): Observable<s
}

const createStorageSettingsCascade: () => Observable<SettingsCascade> = () => {
/** Observable of the JSONC string of the settings.
/**
* Observable of the JSONC string of the settings.
*
* NOTE: We can't use LocalStorageSubject here because the JSONC string is stored raw in localStorage and LocalStorageSubject also does parsing.
* This could be changed, but users already have settings stored, so it would need a migration for little benefit.
Expand Down Expand Up @@ -95,9 +96,8 @@ export function mergeCascades(
}
}

// This is a fragment on the DEPRECATED GraphQL API type ConfigurationCascade (not SettingsCascade) for backcompat.
const configurationCascadeFragment = gql`
fragment ConfigurationCascadeFields on ConfigurationCascade {
const settingsCascadeFragment = gql`
fragment SettingsCascadeFields on SettingsCascade {
subjects {
__typename
...OrgSettingFields
Expand Down Expand Up @@ -167,42 +167,40 @@ const configurationCascadeFragment = gql`

/**
* Fetches the settings cascade for the viewer.
*
* TODO(sqs): This uses the DEPRECATED GraphQL Query.viewerConfiguration and ConfigurationCascade for backcompat.
*/
export function fetchViewerSettings(requestGraphQL: PlatformContext['requestGraphQL']): Observable<{
final: string
subjects: SettingsSubject[]
}> {
return from(
requestGraphQL<ViewerConfigurationResult>({
requestGraphQL<ViewerSettingsResult>({
request: gql`
query ViewerConfiguration {
viewerConfiguration {
...ConfigurationCascadeFields
query ViewerSettings {
viewerSettings {
...SettingsCascadeFields
Comment on lines +179 to +180
Copy link
Member

@eseliger eseliger Jul 19, 2024

Choose a reason for hiding this comment

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

you said in the description

There are no usages of the old API in our own code

But this feels like a usage site?

The browser extension probably needs to be released (manual process I believe, not continuous) for this to go out, otherwise it will probably be broken? 🤔

Copy link
Member Author

@sqs sqs Jul 19, 2024

Choose a reason for hiding this comment

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

Ah yeah, you are right. Thank you for catching this. Confirmed the browser extension hits the viewerConfiguration API.

I'll release a new bext build first before merging this.

image

Copy link
Member

@eseliger eseliger Jul 19, 2024

Choose a reason for hiding this comment

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

Thank you!

}
}
${configurationCascadeFragment}
${settingsCascadeFragment}
`,
variables: {},
mightContainPrivateInfo: false,
})
).pipe(
map(dataOrThrowErrors),
map(({ viewerConfiguration }) => {
if (!viewerConfiguration) {
throw new Error('fetchViewerSettings: empty viewerConfiguration')
map(({ viewerSettings }) => {
if (!viewerSettings) {
throw new Error('fetchViewerSettings: empty viewerSettings')
}

for (const subject of viewerConfiguration.subjects) {
for (const subject of viewerSettings.subjects) {
// User/org/global settings cannot be edited from the
// browser extension (only client settings can).
subject.viewerCanAdminister = false
}

return {
subjects: viewerConfiguration.subjects,
final: viewerConfiguration.merged.contents,
subjects: viewerSettings.subjects,
final: viewerSettings.merged.contents,
}
})
)
Expand Down
2 changes: 0 additions & 2 deletions cmd/frontend/graphqlbackend/default_settings.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,5 +47,3 @@ func (r *defaultSettingsResolver) ViewerCanAdminister(_ context.Context) (bool,
func (r *defaultSettingsResolver) SettingsCascade() *settingsCascade {
return &settingsCascade{db: r.db, subject: &settingsSubjectResolver{defaultSettings: r}}
}

func (r *defaultSettingsResolver) ConfigurationCascade() *settingsCascade { return r.SettingsCascade() }
1 change: 0 additions & 1 deletion cmd/frontend/graphqlbackend/graphqlbackend.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,6 @@ var allowedPrometheusFieldNames = map[[2]string]struct{}{
{"Query", "settingsSubject"}: {},
{"Query", "site"}: {},
{"Query", "user"}: {},
{"Query", "viewerConfiguration"}: {},
{"Query", "viewerSettings"}: {},
{"RegistryExtensionConnection", "nodes"}: {},
{"Repository", "cloneInProgress"}: {},
Expand Down
2 changes: 0 additions & 2 deletions cmd/frontend/graphqlbackend/org.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,8 +258,6 @@ func (o *OrgResolver) SettingsCascade() *settingsCascade {
return &settingsCascade{db: o.db, subject: &settingsSubjectResolver{org: o}}
}

func (o *OrgResolver) ConfigurationCascade() *settingsCascade { return o.SettingsCascade() }

func (o *OrgResolver) ViewerPendingInvitation(ctx context.Context) (*organizationInvitationResolver, error) {
if actor := sgactor.FromContext(ctx); actor.IsAuthenticated() {
orgInvitation, err := o.db.OrgInvitations().GetPending(ctx, o.org.ID, actor.UID)
Expand Down
53 changes: 0 additions & 53 deletions cmd/frontend/graphqlbackend/schema.graphql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1750,10 +1750,6 @@ type Query {
"""
viewerSettings: SettingsCascade!
"""
DEPRECATED
"""
viewerConfiguration: ConfigurationCascade! @deprecated(reason: "use viewerSettings instead")
"""
The configuration for clients.
"""
clientConfiguration: ClientConfigurationDetails!
Expand Down Expand Up @@ -6323,13 +6319,6 @@ type User implements Node & SettingsSubject & Namespace {
"""
settingsCascade: SettingsCascade!
"""
DEPRECATED
"""
configurationCascade: ConfigurationCascade!
@deprecated(
reason: "Use settingsCascade instead. This field is a deprecated alias for it and will be removed in a future release."
)
"""
The organizations that this user is a member of.
"""
organizations: OrgConnection!
Expand Down Expand Up @@ -6860,13 +6849,6 @@ type Org implements Node & SettingsSubject & Namespace {
settingsCascade: SettingsCascade!
"""
DEPRECATED
"""
configurationCascade: ConfigurationCascade!
@deprecated(
reason: "Use settingsCascade instead. This field is a deprecated alias for it and will be removed in a future release."
)
"""
DEPRECATED
A pending invitation for the viewer to join this organization, if any.
"""
viewerPendingInvitation: OrganizationInvitation
Expand Down Expand Up @@ -7025,13 +7007,6 @@ type DefaultSettings implements SettingsSubject {
All viewers can access this field.
"""
settingsCascade: SettingsCascade!
"""
DEPRECATED
"""
configurationCascade: ConfigurationCascade!
@deprecated(
reason: "Use settingsCascade instead. This field is a deprecated alias for it and will be removed in a future release."
)
}

"""
Expand Down Expand Up @@ -7089,13 +7064,6 @@ type Site implements SettingsSubject {
"""
settingsCascade: SettingsCascade!
"""
DEPRECATED
"""
configurationCascade: ConfigurationCascade!
@deprecated(
reason: "Use settingsCascade instead. This field is a deprecated alias for it and will be removed in a future release."
)
"""
The URL to the site's settings.
"""
settingsURL: String
Expand Down Expand Up @@ -7682,13 +7650,6 @@ interface SettingsSubject {
that were merged to produce the final merged settings.
"""
settingsCascade: SettingsCascade!
"""
DEPRECATED
"""
configurationCascade: ConfigurationCascade!
@deprecated(
reason: "Use settingsCascade instead. This field is a deprecated alias for it and will be removed in a future release."
)
}

"""
Expand All @@ -7712,20 +7673,6 @@ type SettingsCascade {
merged: Configuration! @deprecated(reason: "use final instead")
}

"""
DEPRECATED: Renamed to SettingsCascade.
"""
type ConfigurationCascade {
"""
DEPRECATED
"""
subjects: [SettingsSubject!]! @deprecated(reason: "use SettingsCascade.subjects instead")
"""
DEPRECATED
"""
merged: Configuration! @deprecated(reason: "use SettingsCascade.final instead")
}

"""
Settings is a version of a configuration settings file.
"""
Expand Down
7 changes: 1 addition & 6 deletions cmd/frontend/graphqlbackend/settings_cascade.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import (
"github.com/sourcegraph/sourcegraph/internal/trace"
)

// settingsCascade implements the GraphQL type SettingsCascade (and the deprecated type ConfigurationCascade).
// settingsCascade implements the GraphQL type SettingsCascade.
//
// It resolves settings from multiple sources. When there is overlap between values, they will be
// merged in the following cascading order (first is lowest precedence):
Expand Down Expand Up @@ -66,8 +66,3 @@ func (r *schemaResolver) ViewerSettings(ctx context.Context) (*settingsCascade,
}
return &settingsCascade{db: r.db, subject: &settingsSubjectResolver{user: user}}, nil
}

// Deprecated: in the GraphQL API
func (r *schemaResolver) ViewerConfiguration(ctx context.Context) (*settingsCascade, error) {
return newSchemaResolver(r.db, r.gitserverClient).ViewerSettings(ctx)
}
4 changes: 0 additions & 4 deletions cmd/frontend/graphqlbackend/settings_subject.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,3 @@ func (s *settingsSubjectResolver) SettingsCascade() (*settingsCascade, error) {
return nil, errUnknownSettingsSubject
}
}

func (s *settingsSubjectResolver) ConfigurationCascade() (*settingsCascade, error) {
return s.SettingsCascade()
}
2 changes: 0 additions & 2 deletions cmd/frontend/graphqlbackend/site.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,8 +163,6 @@ func (r *siteResolver) SettingsCascade() *settingsCascade {
return &settingsCascade{db: r.db, subject: &settingsSubjectResolver{site: r}}
}

func (r *siteResolver) ConfigurationCascade() *settingsCascade { return r.SettingsCascade() }

func (r *siteResolver) SettingsURL() *string { return strptr("/site-admin/global-settings") }

func (r *siteResolver) CanReloadSite(ctx context.Context) bool {
Expand Down
2 changes: 0 additions & 2 deletions cmd/frontend/graphqlbackend/user.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,8 +400,6 @@ func (r *UserResolver) SettingsCascade() *settingsCascade {
return &settingsCascade{db: r.db, subject: &settingsSubjectResolver{user: r}}
}

func (r *UserResolver) ConfigurationCascade() *settingsCascade { return r.SettingsCascade() }

func (r *UserResolver) SiteAdmin() (bool, error) {
// 🚨 SECURITY: Only the user and admins are allowed to determine if the user is a site admin.
if err := auth.CheckSiteAdminOrSameUserFromActor(r.actor, r.db, r.user.ID); err != nil {
Expand Down