Skip to content
This repository was archived by the owner on Nov 6, 2018. It is now read-only.

Add search features API #95

Merged
merged 17 commits into from
Oct 26, 2018
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
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
47 changes: 47 additions & 0 deletions src/client/api/search.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
import { from, Observable, Subscription } from 'rxjs'
import { ExtSearch } from 'src/extension/api/search'
import { Connection } from 'src/protocol/jsonrpc2/connection'
import { createProxyAndHandleRequests } from '../../common/proxy'
import { TransformQuerySignature } from '../providers/queryTransformer'
import { FeatureProviderRegistry } from '../providers/registry'
import { SubscriptionMap } from './common'

/** @internal */
export interface SearchAPI {
Copy link
Member

Choose a reason for hiding this comment

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

Add /** @internal */ tsdoc directives as in https://sourcegraph.com/github.com/sourcegraph/sourcegraph-extension-api/-/blob/src/client/api/configuration.ts#L8:1 and other files because this type is part of the internal API (so we want to communicate that to readers and to tools like doc generators).

$registerQueryTransformer(id: number): void
$unregister(id: number): void
}

/** @internal */
export class Search implements SearchAPI {
private subscriptions = new Subscription()
private registrations = new SubscriptionMap()
private proxy: ExtSearch

constructor(
connection: Connection,
private queryTransformerRegistry: FeatureProviderRegistry<{}, TransformQuerySignature>
) {
this.subscriptions.add(this.registrations)

this.proxy = createProxyAndHandleRequests('search', connection, this)
}

public $registerQueryTransformer(id: number): void {
this.registrations.add(
id,
this.queryTransformerRegistry.registerProvider(
{},
(query: string): Observable<string> => from(this.proxy.$transformQuery(id, query))
)
)
}

public $unregister(id: number): void {
this.registrations.remove(id)
}

public unsubscribe(): void {
this.subscriptions.unsubscribe()
}
}
2 changes: 2 additions & 0 deletions src/client/controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import { ClientConfiguration } from './api/configuration'
import { ClientContext } from './api/context'
import { ClientDocuments } from './api/documents'
import { ClientLanguageFeatures } from './api/languageFeatures'
import { Search } from './api/search'
import { ClientWindows } from './api/windows'
import { applyContextUpdate, EMPTY_CONTEXT } from './context/context'
import { EMPTY_ENVIRONMENT, Environment } from './environment'
Expand Down Expand Up @@ -268,6 +269,7 @@ export class Controller<X extends Extension, C extends ConfigurationCascade> imp
this.registries.textDocumentReferences
)
)
subscription.add(new Search(client, this.registries.queryTransformer))
subscription.add(new ClientCommands(client, this.registries.commands))
}

Expand Down
70 changes: 70 additions & 0 deletions src/client/providers/queryTransformer.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
import * as assert from 'assert'
import { of } from 'rxjs'
import { TestScheduler } from 'rxjs/testing'
import { transformQuery, TransformQuerySignature } from './queryTransformer'

const scheduler = () => new TestScheduler((a, b) => assert.deepStrictEqual(a, b))

const FIXTURE_INPUT = 'foo'
const FIXTURE_RESULT = 'bar'
const FIXTURE_RESULT_TWO = 'qux'
const FIXTURE_RESULT_MERGED = 'foo bar qux'

describe('transformQuery', () => {
describe('0 providers', () => {
it('returns original query', () =>
scheduler().run(({ cold, expectObservable }) =>
expectObservable(
transformQuery(cold<TransformQuerySignature[]>('-a-|', { a: [] }), FIXTURE_INPUT)
).toBe('-a-|', {
a: FIXTURE_INPUT,
})
))
})

describe('1 provider', () => {
it('returns result from provider', () =>
scheduler().run(({ cold, expectObservable }) =>
expectObservable(
transformQuery(
cold<TransformQuerySignature[]>('-a-|', {
a: [q => of(FIXTURE_RESULT)],
}),
FIXTURE_INPUT
)
).toBe('-a-|', { a: FIXTURE_RESULT })
))
})

describe('2 providers', () => {
it('returns a single query transformed by both providers', () =>
scheduler().run(({ cold, expectObservable }) =>
expectObservable(
transformQuery(
cold<TransformQuerySignature[]>('-a-|', {
a: [q => of(`${q} ${FIXTURE_RESULT}`), q => of(`${q} ${FIXTURE_RESULT_TWO}`)],
}),
FIXTURE_INPUT
)
).toBe('-a-|', { a: FIXTURE_RESULT_MERGED })
))
})

describe('Multiple emissions', () => {
it('returns stream of results', () =>
scheduler().run(({ cold, expectObservable }) =>
expectObservable(
transformQuery(
cold<TransformQuerySignature[]>('-a-b-|', {
a: [q => of(`${q} ${FIXTURE_RESULT}`)],
b: [q => of(`${q} ${FIXTURE_RESULT_TWO}`)],
}),
FIXTURE_INPUT
)
).toBe('-a-b-|', {
a: `${FIXTURE_INPUT} ${FIXTURE_RESULT}`,
b: `${FIXTURE_INPUT} ${FIXTURE_RESULT_TWO}`,
})
))
})
})
34 changes: 34 additions & 0 deletions src/client/providers/queryTransformer.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import { Observable, of } from 'rxjs'
import { flatMap, map, switchMap } from 'rxjs/operators'
import { FeatureProviderRegistry } from './registry'

export type TransformQuerySignature = (query: string) => Observable<string>

/** Transforms search queries using registered query transformers from extensions. */
export class QueryTransformerRegistry extends FeatureProviderRegistry<{}, TransformQuerySignature> {
public transformQuery(query: string): Observable<string> {
return transformQuery(this.providers, query)
}
}

/**
* Returns an observable that emits a query transformed by all providers whenever any of the last-emitted set of providers emits
* a query.
*
* Most callers should use QueryTransformerRegistry's transformQuery method, which uses the registered query transformers
*
*/
export function transformQuery(providers: Observable<TransformQuerySignature[]>, query: string): Observable<string> {
return providers.pipe(
switchMap(providers => {
if (providers.length === 0) {
return [query]
}
return providers.reduce(
(currentQuery, transformQuery) =>
currentQuery.pipe(flatMap(q => transformQuery(q).pipe(map(transformedQuery => transformedQuery)))),
of(query)
)
})
)
}
2 changes: 2 additions & 0 deletions src/client/registries.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { ContributionRegistry } from './providers/contribution'
import { TextDocumentDecorationProviderRegistry } from './providers/decoration'
import { TextDocumentHoverProviderRegistry } from './providers/hover'
import { TextDocumentLocationProviderRegistry, TextDocumentReferencesProviderRegistry } from './providers/location'
import { QueryTransformerRegistry } from './providers/queryTransformer'

/**
* Registries is a container for all provider registries.
Expand All @@ -25,4 +26,5 @@ export class Registries<X extends Extension, C extends ConfigurationCascade> {
public readonly textDocumentTypeDefinition = new TextDocumentLocationProviderRegistry()
public readonly textDocumentHover = new TextDocumentHoverProviderRegistry()
public readonly textDocumentDecoration = new TextDocumentDecorationProviderRegistry()
public readonly queryTransformer = new QueryTransformerRegistry()
}
24 changes: 24 additions & 0 deletions src/extension/api/search.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import { Unsubscribable } from 'rxjs'
import { QueryTransformer } from 'sourcegraph'
import { SearchAPI } from 'src/client/api/search'
import { ProviderMap } from './common'

export interface ExtSearchAPI {
$transformQuery: (id: number, query: string) => Promise<string>
}

export class ExtSearch implements ExtSearchAPI {
private registrations = new ProviderMap<QueryTransformer>(id => this.proxy.$unregister(id))
constructor(private proxy: SearchAPI) {}

public registerQueryTransformer(provider: QueryTransformer): Unsubscribable {
const { id, subscription } = this.registrations.add(provider)
this.proxy.$registerQueryTransformer(id)
return subscription
}

public $transformQuery(id: number, query: string): Promise<string> {
const provider = this.registrations.get<QueryTransformer>(id)
return Promise.resolve(provider.transformQuery(query))
}
}
8 changes: 8 additions & 0 deletions src/extension/extensionHost.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { ExtConfiguration } from './api/configuration'
import { ExtContext } from './api/context'
import { ExtDocuments } from './api/documents'
import { ExtLanguageFeatures } from './api/languageFeatures'
import { ExtSearch } from './api/search'
import { ExtWindows } from './api/windows'
import { Location } from './types/location'
import { Position } from './types/position'
Expand Down Expand Up @@ -82,6 +83,9 @@ function createExtensionHandle(initData: InitData, connection: Connection): type
const languageFeatures = new ExtLanguageFeatures(proxy('languageFeatures'), documents)
handleRequests(connection, 'languageFeatures', languageFeatures)

const search = new ExtSearch(proxy('search'))
handleRequests(connection, 'search', search)

const commands = new ExtCommands(proxy('commands'))
handleRequests(connection, 'commands', commands)

Expand Down Expand Up @@ -129,6 +133,10 @@ function createExtensionHandle(initData: InitData, connection: Connection): type
languageFeatures.registerReferenceProvider(selector, provider),
},

search: {
registerQueryTransformer: provider => search.registerQueryTransformer(provider),
},

commands: {
registerCommand: (command, callback) => commands.registerCommand({ command, callback }),
executeCommand: (command, ...args) => commands.executeCommand(command, args),
Expand Down
46 changes: 46 additions & 0 deletions src/integration-test/search.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import * as assert from 'assert'
import { take } from 'rxjs/operators'
import { integrationTestContext } from './helpers.test'

describe('search (integration)', () => {
it('registers a query transformer', async () => {
const { clientController, extensionHost, ready } = await integrationTestContext()

// Register the provider and call it
const unsubscribe = extensionHost.search.registerQueryTransformer({ transformQuery: () => 'bar' })
await ready
assert.deepStrictEqual(
await clientController.registries.queryTransformer
.transformQuery('foo')
.pipe(take(1))
.toPromise(),
'bar'
)

// Unregister the provider and ensure it's removed.
unsubscribe.unsubscribe()
assert.deepStrictEqual(
await clientController.registries.queryTransformer
.transformQuery('foo')
.pipe(take(1))
.toPromise(),
'foo'
)
})

it('supports multiple query transformers', async () => {
const { clientController, extensionHost, ready } = await integrationTestContext()

// Register the provider and call it
extensionHost.search.registerQueryTransformer({ transformQuery: q => `${q} bar` })
Copy link
Member

Choose a reason for hiding this comment

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

Minor, but bar and baz look almost the same; it's not easy to visually spot that they are distinct, given that everything else on the 2 lines is the same. I would recommend using foo, qux, etc., here on one of the lines so it's easier to spot that they are different.

extensionHost.search.registerQueryTransformer({ transformQuery: q => `${q} qux` })
await ready
assert.deepStrictEqual(
await clientController.registries.queryTransformer
.transformQuery('foo')
.pipe(take(1))
.toPromise(),
'foo bar qux'
)
})
})
31 changes: 31 additions & 0 deletions src/sourcegraph.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -785,6 +785,37 @@ declare module 'sourcegraph' {
): Unsubscribable
}

/**
* A query transformer alters a user's search query before executing a search.
*
* Query transformers allow extensions to define new search query operators and syntax, for example,
* by matching strings in a query (e.g. `go.imports:`) and replacing them with a regular expression or string.
*/
export interface QueryTransformer {
/**
* Transforms a search query into another, valid query. If there are no transformations to be made
Copy link
Member

Choose a reason for hiding this comment

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

I'd replace "the original query is returned" with ", it should return the original query"

* the original query is returned.
*
* @param query A search query provided by a client.
Copy link
Member

Choose a reason for hiding this comment

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

just "A search query" is fine, no need to say "provided by a client" (not sure what that adds)

*/
transformQuery(query: string): string | Promise<string>
}

/**
* API for extensions to augment search functionality.
*/
export namespace search {
/**
* Registers a query transformer.
*
* Multiple transformers can be registered. In that case, all transformers will be executed
Copy link
Member

Choose a reason for hiding this comment

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

I think it is more idiomatic to say a transformation is applied, not executed. So replace "executed" with "applied".

Also, the order in which multiple transformers are applied is undefined (i.e., the extension API makes no guarantee). So I'd state that.

* and the result is a single query that has been altered by all transformers.
*
* @param provider A query transformer.
*/
export function registerQueryTransformer(provider: QueryTransformer): Unsubscribable
}

/**
* Commands are functions that are implemented and registered by extensions. Extensions can invoke any command
* (including commands registered by other extensions). The extension can also define contributions (in
Expand Down