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.

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

[maps] Use label features from ES vector tile search API to fix multiple labels #132080

Merged
merged 15 commits into from
May 20, 2022
Merged
6 changes: 6 additions & 0 deletions x-pack/plugins/maps/common/mvt_request_body.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ export function getAggsTileRequest({
encodedRequestBody,
geometryFieldName,
gridPrecision,
hasLabels,
index,
renderAs = RENDER_AS.POINT,
x,
Expand All @@ -30,6 +31,7 @@ export function getAggsTileRequest({
encodedRequestBody: string;
geometryFieldName: string;
gridPrecision: number;
hasLabels: boolean;
index: string;
renderAs: RENDER_AS;
x: number;
Expand All @@ -50,20 +52,23 @@ export function getAggsTileRequest({
aggs: requestBody.aggs,
fields: requestBody.fields,
runtime_mappings: requestBody.runtime_mappings,
with_labels: hasLabels,
},
};
}

export function getHitsTileRequest({
encodedRequestBody,
geometryFieldName,
hasLabels,
index,
x,
y,
z,
}: {
encodedRequestBody: string;
geometryFieldName: string;
hasLabels: boolean;
index: string;
x: number;
y: number;
Expand All @@ -86,6 +91,7 @@ export function getHitsTileRequest({
),
runtime_mappings: requestBody.runtime_mappings,
track_total_hits: typeof requestBody.size === 'number' ? requestBody.size + 1 : false,
with_labels: hasLabels,
},
};
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ export class HeatmapLayer extends AbstractLayer {

async syncData(syncContext: DataRequestContext) {
await syncMvtSourceData({
hasLabels: false,
layerId: this.getId(),
layerName: await this.getDisplayName(),
prevDataRequest: this.getSourceDataRequest(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ describe('syncMvtSourceData', () => {
const syncContext = new MockSyncContext({ dataFilters: {} });

await syncMvtSourceData({
hasLabels: false,
layerId: 'layer1',
layerName: 'my layer',
prevDataRequest: undefined,
Expand Down Expand Up @@ -82,6 +83,7 @@ describe('syncMvtSourceData', () => {
tileSourceLayer: 'aggs',
tileUrl: 'https://example.com/{x}/{y}/{z}.pbf',
refreshToken: '12345',
hasLabels: false,
});
});

Expand All @@ -99,6 +101,7 @@ describe('syncMvtSourceData', () => {
};

await syncMvtSourceData({
hasLabels: false,
layerId: 'layer1',
layerName: 'my layer',
prevDataRequest: {
Expand All @@ -112,6 +115,7 @@ describe('syncMvtSourceData', () => {
tileSourceLayer: 'aggs',
tileUrl: 'https://example.com/{x}/{y}/{z}.pbf?token=12345',
refreshToken: '12345',
hasLabels: false,
};
},
} as unknown as DataRequest,
Expand Down Expand Up @@ -142,6 +146,7 @@ describe('syncMvtSourceData', () => {
};

await syncMvtSourceData({
hasLabels: false,
layerId: 'layer1',
layerName: 'my layer',
prevDataRequest: {
Expand All @@ -155,6 +160,7 @@ describe('syncMvtSourceData', () => {
tileSourceLayer: 'aggs',
tileUrl: 'https://example.com/{x}/{y}/{z}.pbf?token=12345',
refreshToken: '12345',
hasLabels: false,
};
},
} as unknown as DataRequest,
Expand Down Expand Up @@ -182,6 +188,7 @@ describe('syncMvtSourceData', () => {
};

await syncMvtSourceData({
hasLabels: false,
layerId: 'layer1',
layerName: 'my layer',
prevDataRequest: {
Expand All @@ -195,6 +202,7 @@ describe('syncMvtSourceData', () => {
tileSourceLayer: 'aggs',
tileUrl: 'https://example.com/{x}/{y}/{z}.pbf?token=12345',
refreshToken: '12345',
hasLabels: false,
};
},
} as unknown as DataRequest,
Expand Down Expand Up @@ -230,6 +238,7 @@ describe('syncMvtSourceData', () => {
};

await syncMvtSourceData({
hasLabels: false,
layerId: 'layer1',
layerName: 'my layer',
prevDataRequest: {
Expand All @@ -243,6 +252,7 @@ describe('syncMvtSourceData', () => {
tileSourceLayer: 'barfoo', // tileSourceLayer is different then mockSource
tileUrl: 'https://example.com/{x}/{y}/{z}.pbf?token=12345',
refreshToken: '12345',
hasLabels: false,
};
},
} as unknown as DataRequest,
Expand Down Expand Up @@ -270,6 +280,7 @@ describe('syncMvtSourceData', () => {
};

await syncMvtSourceData({
hasLabels: false,
layerId: 'layer1',
layerName: 'my layer',
prevDataRequest: {
Expand All @@ -283,6 +294,7 @@ describe('syncMvtSourceData', () => {
tileSourceLayer: 'aggs',
tileUrl: 'https://example.com/{x}/{y}/{z}.pbf?token=12345',
refreshToken: '12345',
hasLabels: false,
};
},
} as unknown as DataRequest,
Expand Down Expand Up @@ -310,6 +322,7 @@ describe('syncMvtSourceData', () => {
};

await syncMvtSourceData({
hasLabels: false,
layerId: 'layer1',
layerName: 'my layer',
prevDataRequest: {
Expand All @@ -323,6 +336,49 @@ describe('syncMvtSourceData', () => {
tileSourceLayer: 'aggs',
tileUrl: 'https://example.com/{x}/{y}/{z}.pbf?token=12345',
refreshToken: '12345',
hasLabels: false,
};
},
} as unknown as DataRequest,
requestMeta: { ...prevRequestMeta },
source: mockSource,
syncContext,
});
// @ts-expect-error
sinon.assert.calledOnce(syncContext.startLoading);
// @ts-expect-error
sinon.assert.calledOnce(syncContext.stopLoading);
});

test('Should re-sync when hasLabel state changes', async () => {
const syncContext = new MockSyncContext({ dataFilters: {} });
const prevRequestMeta = {
...syncContext.dataFilters,
applyGlobalQuery: true,
applyGlobalTime: true,
applyForceRefresh: true,
fieldNames: [],
sourceMeta: {},
isForceRefresh: false,
isFeatureEditorOpenForLayer: false,
};

await syncMvtSourceData({
hasLabels: true,
layerId: 'layer1',
layerName: 'my layer',
prevDataRequest: {
getMeta: () => {
return prevRequestMeta;
},
getData: () => {
return {
tileMinZoom: 4,
tileMaxZoom: 14,
tileSourceLayer: 'aggs',
tileUrl: 'https://example.com/{x}/{y}/{z}.pbf?token=12345',
refreshToken: '12345',
hasLabels: false,
};
},
} as unknown as DataRequest,
Expand All @@ -340,6 +396,7 @@ describe('syncMvtSourceData', () => {
const syncContext = new MockSyncContext({ dataFilters: {} });

await syncMvtSourceData({
hasLabels: false,
layerId: 'layer1',
layerName: 'my layer',
prevDataRequest: undefined,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,19 @@ export interface MvtSourceData {
tileMaxZoom: number;
tileUrl: string;
refreshToken: string;
hasLabels: boolean;
}

export async function syncMvtSourceData({
hasLabels,
layerId,
layerName,
prevDataRequest,
requestMeta,
source,
syncContext,
}: {
hasLabels: boolean;
layerId: string;
layerName: string;
prevDataRequest: DataRequest | undefined;
Expand All @@ -56,7 +59,10 @@ export async function syncMvtSourceData({
},
});
const canSkip =
!syncContext.forceRefreshDueToDrawing && noChangesInSourceState && noChangesInSearchState;
!syncContext.forceRefreshDueToDrawing &&
noChangesInSourceState &&
noChangesInSearchState &&
prevData.hasLabels === hasLabels;

if (canSkip) {
return;
Expand All @@ -72,7 +78,7 @@ export async function syncMvtSourceData({
? uuid()
: prevData.refreshToken;

const tileUrl = await source.getTileUrl(requestMeta, refreshToken);
const tileUrl = await source.getTileUrl(requestMeta, refreshToken, hasLabels);
if (source.isESSource()) {
syncContext.inspectorAdapters.vectorTiles.addLayer(layerId, layerName, tileUrl);
}
Expand All @@ -82,6 +88,7 @@ export async function syncMvtSourceData({
tileMinZoom: source.getMinZoom(),
tileMaxZoom: source.getMaxZoom(),
refreshToken,
hasLabels,
};
syncContext.stopLoading(SOURCE_DATA_REQUEST_ID, requestToken, sourceData, {});
} catch (error) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,7 @@ export class MvtVectorLayer extends AbstractVectorLayer {
await this._syncSupportsFeatureEditing({ syncContext, source: this.getSource() });

await syncMvtSourceData({
hasLabels: this.getCurrentStyle().hasLabels(),
layerId: this.getId(),
layerName: await this.getDisplayName(),
prevDataRequest: this.getSourceDataRequest(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -736,7 +736,10 @@ export class AbstractVectorLayer extends AbstractLayer implements IVectorLayer {
}
}

const isSourceGeoJson = !this.getSource().isMvt();
const filterExpr = getPointFilterExpression(
isSourceGeoJson,
this.getSource().isESSource(),
this._getJoinFilterExpression(),
timesliceMaskConfig
);
Expand Down Expand Up @@ -843,6 +846,7 @@ export class AbstractVectorLayer extends AbstractLayer implements IVectorLayer {
const isSourceGeoJson = !this.getSource().isMvt();
const filterExpr = getLabelFilterExpression(
isSourceGeoJson,
this.getSource().isESSource(),
this._getJoinFilterExpression(),
timesliceMaskConfig
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -306,10 +306,10 @@ describe('ESGeoGridSource', () => {
});

it('getTileUrl', async () => {
const tileUrl = await mvtGeogridSource.getTileUrl(vectorSourceRequestMeta, '1234');
const tileUrl = await mvtGeogridSource.getTileUrl(vectorSourceRequestMeta, '1234', false);

expect(tileUrl).toEqual(
"rootdir/api/maps/mvt/getGridTile/{z}/{x}/{y}.pbf?geometryFieldName=bar&index=undefined&gridPrecision=8&requestBody=(foobar%3AES_DSL_PLACEHOLDER%2Cparams%3A('0'%3A('0'%3Aindex%2C'1'%3A(fields%3A()))%2C'1'%3A('0'%3Asize%2C'1'%3A0)%2C'2'%3A('0'%3Afilter%2C'1'%3A!())%2C'3'%3A('0'%3Aquery)%2C'4'%3A('0'%3Aindex%2C'1'%3A(fields%3A()))%2C'5'%3A('0'%3Aquery%2C'1'%3A(language%3AKQL%2Cquery%3A''))%2C'6'%3A('0'%3Aaggs%2C'1'%3A())))&renderAs=heatmap&token=1234"
"rootdir/api/maps/mvt/getGridTile/{z}/{x}/{y}.pbf?geometryFieldName=bar&index=undefined&gridPrecision=8&hasLabels=false&requestBody=(foobar%3AES_DSL_PLACEHOLDER%2Cparams%3A('0'%3A('0'%3Aindex%2C'1'%3A(fields%3A()))%2C'1'%3A('0'%3Asize%2C'1'%3A0)%2C'2'%3A('0'%3Afilter%2C'1'%3A!())%2C'3'%3A('0'%3Aquery)%2C'4'%3A('0'%3Aindex%2C'1'%3A(fields%3A()))%2C'5'%3A('0'%3Aquery%2C'1'%3A(language%3AKQL%2Cquery%3A''))%2C'6'%3A('0'%3Aaggs%2C'1'%3A())))&renderAs=heatmap&token=1234"
);
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -471,7 +471,11 @@ export class ESGeoGridSource extends AbstractESAggSource implements IMvtVectorSo
return 'aggs';
}

async getTileUrl(searchFilters: VectorSourceRequestMeta, refreshToken: string): Promise<string> {
async getTileUrl(
searchFilters: VectorSourceRequestMeta,
refreshToken: string,
hasLabels: boolean
): Promise<string> {
const indexPattern = await this.getIndexPattern();
const searchSource = await this.makeSearchSource(searchFilters, 0);
searchSource.setField('aggs', this.getValueAggsDsl(indexPattern));
Expand All @@ -484,6 +488,7 @@ export class ESGeoGridSource extends AbstractESAggSource implements IMvtVectorSo
?geometryFieldName=${this._descriptor.geoField}\
&index=${indexPattern.title}\
&gridPrecision=${this._getGeoGridPrecisionResolutionDelta()}\
&hasLabels=${hasLabels}\
&requestBody=${encodeMvtResponseBody(searchSource.getSearchRequestBody())}\
&renderAs=${this._descriptor.requestType}\
&token=${refreshToken}`;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,9 +114,9 @@ describe('ESSearchSource', () => {
geoField: geoFieldName,
indexPatternId: 'ipId',
});
const tileUrl = await esSearchSource.getTileUrl(searchFilters, '1234');
const tileUrl = await esSearchSource.getTileUrl(searchFilters, '1234', false);
expect(tileUrl).toBe(
`rootdir/api/maps/mvt/getTile/{z}/{x}/{y}.pbf?geometryFieldName=bar&index=foobar-title-*&requestBody=(foobar%3AES_DSL_PLACEHOLDER%2Cparams%3A('0'%3A('0'%3Aindex%2C'1'%3A(fields%3A()%2Ctitle%3A'foobar-title-*'))%2C'1'%3A('0'%3Asize%2C'1'%3A1000)%2C'2'%3A('0'%3Afilter%2C'1'%3A!())%2C'3'%3A('0'%3Aquery)%2C'4'%3A('0'%3Aindex%2C'1'%3A(fields%3A()%2Ctitle%3A'foobar-title-*'))%2C'5'%3A('0'%3Aquery%2C'1'%3A(language%3AKQL%2Cquery%3A'tooltipField%3A%20foobar'))%2C'6'%3A('0'%3AfieldsFromSource%2C'1'%3A!(tooltipField%2CstyleField))%2C'7'%3A('0'%3Asource%2C'1'%3A!(tooltipField%2CstyleField))))&token=1234`
`rootdir/api/maps/mvt/getTile/{z}/{x}/{y}.pbf?geometryFieldName=bar&index=foobar-title-*&hasLabels=false&requestBody=(foobar%3AES_DSL_PLACEHOLDER%2Cparams%3A('0'%3A('0'%3Aindex%2C'1'%3A(fields%3A()%2Ctitle%3A'foobar-title-*'))%2C'1'%3A('0'%3Asize%2C'1'%3A1000)%2C'2'%3A('0'%3Afilter%2C'1'%3A!())%2C'3'%3A('0'%3Aquery)%2C'4'%3A('0'%3Aindex%2C'1'%3A(fields%3A()%2Ctitle%3A'foobar-title-*'))%2C'5'%3A('0'%3Aquery%2C'1'%3A(language%3AKQL%2Cquery%3A'tooltipField%3A%20foobar'))%2C'6'%3A('0'%3AfieldsFromSource%2C'1'%3A!(tooltipField%2CstyleField))%2C'7'%3A('0'%3Asource%2C'1'%3A!(tooltipField%2CstyleField))))&token=1234`
);
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -810,7 +810,11 @@ export class ESSearchSource extends AbstractESSource implements IMvtVectorSource
return 'hits';
}

async getTileUrl(searchFilters: VectorSourceRequestMeta, refreshToken: string): Promise<string> {
async getTileUrl(
searchFilters: VectorSourceRequestMeta,
refreshToken: string,
hasLabels: boolean
): Promise<string> {
const indexPattern = await this.getIndexPattern();
const indexSettings = await loadIndexSettings(indexPattern.title);

Expand Down Expand Up @@ -847,6 +851,7 @@ export class ESSearchSource extends AbstractESSource implements IMvtVectorSource
return `${mvtUrlServicePath}\
?geometryFieldName=${this._descriptor.geoField}\
&index=${indexPattern.title}\
&hasLabels=${hasLabels}\
&requestBody=${encodeMvtResponseBody(searchSource.getSearchRequestBody())}\
&token=${refreshToken}`;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,11 @@ export interface IMvtVectorSource extends IVectorSource {
* IMvtVectorSource.getTileUrl returns the tile source URL.
* Append refreshToken as a URL parameter to force tile re-fetch on refresh (not required)
*/
getTileUrl(searchFilters: VectorSourceRequestMeta, refreshToken: string): Promise<string>;
getTileUrl(
searchFilters: VectorSourceRequestMeta,
refreshToken: string,
hasLabels: boolean
): Promise<string>;

/*
* Tile vector sources can contain multiple layers. For example, elasticsearch _mvt tiles contain the layers "hits", "aggs", and "meta".
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,9 +94,9 @@ export function makeMbClampedNumberExpression({
];
}

export function getHasLabel(label: StaticTextProperty | DynamicTextProperty) {
export function getHasLabel(label: StaticTextProperty | DynamicTextProperty): boolean {
return label.isDynamic()
? label.isComplete()
: (label as StaticTextProperty).getOptions().value != null &&
(label as StaticTextProperty).getOptions().value.length;
(label as StaticTextProperty).getOptions().value.length > 0;
nreese marked this conversation as resolved.
Show resolved Hide resolved
}
Loading