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

Antimeridian and bbox calc fix #1115

Merged
merged 7 commits into from
Jun 1, 2022
Merged
Show file tree
Hide file tree
Changes from all 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
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ export class MemoryExportRepo implements ExportRepository {

async findLatestExportsFor(
projectId: string,
limit: number = 5,
limit = 5,
options?: {
isStandalone?: boolean;
isFinished?: boolean;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ class FakeScenarioRepository {

class FakeExportRepository implements ExportRepository {
public returnUnfinishedExport = false;
public importResourceId: string = '';
public importResourceId = '';

async save(exportInstance: Export): Promise<Either<SaveError, Success>> {
throw new Error('Method not implemented.');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { GeoFeature } from './geo-feature.api.entity';
import { GeoFeaturePropertySet } from './geo-feature.geo.entity';
import { DbConnections } from '@marxan-api/ormconfig.connections';
import { BBox } from 'geojson';
import { antimeridianBbox } from '@marxan/utils/geo';

@Injectable()
export class GeoFeaturePropertySetService {
Expand All @@ -32,16 +33,25 @@ export class GeoFeaturePropertySetService {
.where(`propertySets.featureId IN (:...ids)`, { ids: geoFeatureIds });

if (withinBBox) {
const { westBbox, eastBbox } = antimeridianBbox([
withinBBox[1],
withinBBox[3],
withinBBox[0],
withinBBox[2],
]);
query.andWhere(
`st_intersects(
st_makeenvelope(:xmin, :ymin, :xmax, :ymax, 4326),
"propertySets".bbox
)`,
`(st_intersects(
st_intersection(st_makeenvelope(:...westBbox, 4326),
ST_MakeEnvelope(0, -90, 180, 90, 4326)),
"propertySets".bbox)
or
st_intersects(
st_intersection(st_makeenvelope(:...eastBbox, 4326),
ST_MakeEnvelope(-180, -90, 0, 90, 4326)),
"propertySets".bbox))`,
{
xmin: withinBBox[1],
ymin: withinBBox[3],
xmax: withinBBox[0],
ymax: withinBBox[2],
westBbox: westBbox,
eastBbox: eastBbox,
},
);
}
Expand Down
19 changes: 12 additions & 7 deletions api/apps/api/src/modules/geo-features/geo-features.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import { DbConnections } from '@marxan-api/ormconfig.connections';
import { v4 } from 'uuid';
import { UploadShapefileDTO } from '../projects/dto/upload-shapefile.dto';
import { GeoFeaturesRequestInfo } from './geo-features-request-info';
import { antimeridianBbox, nominatim2bbox } from '@marxan/utils/geo';

const geoFeatureFilterKeyNames = [
'featureClassName',
Expand Down Expand Up @@ -167,20 +168,24 @@ export class GeoFeaturesService extends AppBaseService<
*
*/
if (projectId && info?.params?.bbox) {
const { westBbox, eastBbox } = antimeridianBbox(nominatim2bbox(info.params.bbox));
const geoFeaturesWithinProjectBbox = await this.geoFeaturesGeometriesRepository
.createQueryBuilder('geoFeatureGeometries')
.select('"geoFeatureGeometries"."feature_id"', 'featureId')
.distinctOn(['"geoFeatureGeometries"."feature_id"'])
.where(
`st_intersects(
st_makeenvelope(:xmin, :ymin, :xmax, :ymax, 4326),
`(st_intersects(
st_intersection(st_makeenvelope(:...eastBbox, 4326),
ST_MakeEnvelope(0, -90, 180, 90, 4326)),
Copy link
Member

Choose a reason for hiding this comment

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

no need to do anything in this PR about this, from my point of view, but these could be good candidates for a named constant that allows to avoid "magic numbers" that may take a little while for the inexperienced reader to figure out when they look at this code out of context (for example, not knowing that it was introduced while splitting semihemispheres)

Copy link
Member Author

Choose a reason for hiding this comment

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

yes in the future we will need to revisit some of this, as well as at some point pay the debt in regarding sql formulation

"geoFeatureGeometries".the_geom
)`,
) or st_intersects(
st_intersection(st_makeenvelope(:...westBbox, 4326),
ST_MakeEnvelope(-180, -90, 0, 90, 4326)),
"geoFeatureGeometries".the_geom
))`,
{
xmin: info.params.bbox[1],
ymin: info.params.bbox[3],
xmax: info.params.bbox[0],
ymax: info.params.bbox[2],
westBbox: westBbox,
eastBbox: eastBbox,
},
)
.getRawMany()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ export class MarkLegacyProjectImportPieceAsFailed extends Command<void> {
public readonly projectId: ResourceId,
public readonly legacyProjectImportComponentId: LegacyProjectImportComponentId,
public readonly errors: string[] = [],
public readonly warnings: string[] = [],
) {
super();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ export class MarkLegacyProjectImportPieceAsFailedHandler
errors,
legacyProjectImportComponentId,
projectId,
warnings,
}: MarkLegacyProjectImportPieceAsFailed): Promise<void> {
const result = await this.legacyProjectImportRepository.transaction(
async (repo) => {
Expand All @@ -61,7 +60,6 @@ export class MarkLegacyProjectImportPieceAsFailedHandler
const result = aggregate.markPieceAsFailed(
legacyProjectImportComponentId,
errors,
warnings,
);

if (isLeft(result)) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { ArchiveLocation } from '@marxan/cloning/domain';
import {
LegacyProjectImportPiece,
LegacyProjectImportPieceOrderResolver,
Expand Down Expand Up @@ -49,10 +48,9 @@ export class LegacyProjectImportComponent {
this.warnings.push(...warnings);
}

markAsFailed(errors: string[] = [], warnings: string[] = []) {
markAsFailed(errors: string[] = []) {
this.status = this.status.markAsFailed();
this.errors.push(...errors);
this.warnings.push(...warnings);
}

toSnapshot(): LegacyProjectImportComponentSnapshot {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -190,14 +190,13 @@ export class LegacyProjectImport extends AggregateRoot {
markPieceAsFailed(
pieceId: LegacyProjectImportComponentId,
errors: string[] = [],
warnings: string[] = [],
): Either<MarkLegacyProjectImportPieceAsFailedErrors, true> {
const piece = this.pieces.find((pc) => pc.id.value === pieceId.value);
if (!piece) return left(legacyProjectImportComponentNotFound);
if (piece.hasFailed())
return left(legacyProjectImportComponentAlreadyFailed);

piece.markAsFailed(errors, warnings);
piece.markAsFailed(errors);

const hasThisBatchFinished = this.hasBatchFinished(piece.order);
const hasThisBatchFailed = this.hasBatchFailed(piece.order);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,13 +99,16 @@ export class ImportLegacyProjectPieceEventsHandler
private async failed(event: EventData<LegacyProjectImportJobInput, unknown>) {
const { pieceId, projectId } = await event.data;

const result = await event.result;
const errors: string[] = [];

if (typeof result === 'string') errors.push(result);

await this.commandBus.execute(
new MarkLegacyProjectImportPieceAsFailed(
new ResourceId(projectId),
new LegacyProjectImportComponentId(pieceId),
// TODO Obtain actual errors and/or warnings
[],
[],
errors,
),
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
import { ScheduleDbCleanupForFailedLegacyProjectImportHandler } from './schedule-db-cleanup-for-failed-legacy-project-import.handler';
import { LegacyProjectImportRepositoryModule } from './legacy-project-import.repository.module';
import { ScheduleLegacyProjectImportPieceHandler } from './schedule-legacy-project-import-piece.handler';
import { ImportLegacyProjectPieceEventsHandler } from './import-legacy-project-piece.events-handler';

@Module({
imports: [
Expand All @@ -30,6 +31,7 @@ import { ScheduleLegacyProjectImportPieceHandler } from './schedule-legacy-proje
ScheduleDbCleanupForFailedLegacyProjectImportHandler,
LegacyProjectImportPieceRequestedSaga,
ScheduleLegacyProjectImportPieceHandler,
ImportLegacyProjectPieceEventsHandler,
{
provide: Logger,
useClass: Logger,
Expand Down
12 changes: 8 additions & 4 deletions api/apps/api/src/modules/queue-api-events/adapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,8 @@ export class QueueEventsAdapter<
queueEvents.on(`completed`, ({ jobId }, eventId) =>
this.handleFinished(jobId, eventId),
);
queueEvents.on(`failed`, ({ jobId }, eventId) =>
this.handleFailed(jobId, eventId),
queueEvents.on(`failed`, ({ jobId, failedReason }, eventId) =>
this.handleFailed(jobId, eventId, failedReason),
);
}

Expand Down Expand Up @@ -84,7 +84,11 @@ export class QueueEventsAdapter<
});
}

private async handleFailed(jobId: string, eventId: string) {
private async handleFailed(
jobId: string,
eventId: string,
failedReason: string,
) {
const lazyDataGetter = this.createLazyDataGetter();
const eventDto = await this.eventFactory.createFailedEvent({
jobId,
Expand All @@ -107,7 +111,7 @@ export class QueueEventsAdapter<
return lazyDataGetter(jobId);
},
get result() {
return Promise.resolve(undefined);
return Promise.resolve(failedReason);
},
});
}
Expand Down
2 changes: 1 addition & 1 deletion api/apps/api/src/utils/json.type.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,4 @@ export interface JSONObject {
[x: string]: JSONValue;
}

export interface JSONArray extends Array<JSONValue> {}
export type JSONArray = Array<JSONValue>;
5 changes: 5 additions & 0 deletions api/apps/api/test/fixtures/test-init-apidb.sql
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,8 @@ VALUES
('[email protected]', 'b', 'b', 'User B B', true, false, crypt('bbuserpassword', gen_salt('bf'))),
('[email protected]', 'c', 'c', 'User C C', true, false, crypt('ccuserpassword', gen_salt('bf'))),
('[email protected]', 'd', 'd', 'User D D', true, false, crypt('dduserpassword', gen_salt('bf')));

INSERT INTO organizations (name, created_by)
VALUES
('Example Org 1', (SELECT id FROM users WHERE email = '[email protected]')),
('Example Org 2', (SELECT id FROM users WHERE email = '[email protected]'));
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
import { MigrationInterface, QueryRunner } from 'typeorm';

export class ModifyBboxCalculationTakingAntimeridian1653565019335
implements MigrationInterface {
public async up(queryRunner: QueryRunner): Promise<void> {
await queryRunner.query(`
CREATE OR REPLACE FUNCTION mx_bbox2json(geom geometry)
aagm marked this conversation as resolved.
Show resolved Hide resolved
returns jsonb
language plpgsql
as
$function$
DECLARE
-- variable declaration
both_hemispheres RECORD;
BEGIN
-- logic https://github.com/mapbox/carmen/blob/03fac2d7397ecdfcb4f0828fcfd9d8a54c845f21/lib/util/bbox.js#L59
-- json form of the bbox should be in Nominatim bbox [xmin, xmax, ymin, ymax] [W, E, S, N].
execute 'with region as (select st_intersection($1, geom) as the_geom,
st_intersects($1, geom) intersects, pos
from (values (ST_MakeEnvelope(-180, -90, 0, 90, 4326), ''west''),
(ST_MakeEnvelope(0, -90, 180, 90, 4326), ''east'')) as t(geom, pos)),
data as (select ST_XMax(the_geom), ST_XMin(the_geom),
ST_YMax(the_geom),ST_YMin(the_geom), pos, intersects,
ST_XMax(the_geom) + ABS(lag(ST_XMin(the_geom), 1) OVER ()) >
(180 - ST_XMin(the_geom)) + (180 - ABS(lag(ST_XMax(the_geom), 1) OVER ())) as pm_am
from region)
select bool_and(intersects) and bool_and(pm_am) result,
jsonb_build_array(max(st_xmax), min(st_xmin), max(st_ymax), min(st_ymin)) if_false,
jsonb_build_array(min(st_xmax), max(st_xmin), max(st_ymax), min(st_ymin))if_true from data;'
into both_hemispheres
using geom;
if both_hemispheres.result then
return both_hemispheres.if_true;
else
return both_hemispheres.if_false;
end if;
end;
$function$;

CREATE OR REPLACE FUNCTION public.tr_getbbox()
RETURNS trigger
LANGUAGE plpgsql
AS $function$
BEGIN
NEW.bbox := mx_bbox2json(NEW.the_geom);
RETURN NEW;
END;
$function$;

UPDATE admin_regions SET id = id;
`);
}

public async down(queryRunner: QueryRunner): Promise<void> {
await queryRunner.query(`
CREATE OR REPLACE FUNCTION public.tr_getbbox()
RETURNS trigger
LANGUAGE plpgsql
AS $function$
BEGIN
NEW.bbox := jsonb_build_array(ST_XMax(NEW.the_geom), ST_XMin(NEW.the_geom),
ST_YMax(NEW.the_geom), ST_YMin(NEW.the_geom));
RETURN NEW;
END;
$function$;

Drop FUNCTION mx_bbox2json(geom geometry);

UPDATE admin_regions SET id = id;
`);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import {
import { Transform } from 'class-transformer';
import { BBox } from 'geojson';
import { AdminArea } from '@marxan/admin-regions';
import { nominatim2bbox } from '@marxan-geoprocessing/utils/bbox.utils';
import { nominatim2bbox } from '@marxan/utils/geo';
import { TileRequest } from '@marxan/tiles';

export class TileSpecification extends TileRequest {
Expand Down
20 changes: 15 additions & 5 deletions api/apps/geoprocessing/src/modules/features/features.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { IsArray, IsNumber, IsString, IsOptional } from 'class-validator';
import { ApiProperty } from '@nestjs/swagger';
import { Transform } from 'class-transformer';
import { BBox } from 'geojson';
import { nominatim2bbox } from '@marxan-geoprocessing/utils/bbox.utils';
import { antimeridianBbox, nominatim2bbox } from '@marxan/utils/geo';

import { TileRequest } from '@marxan/tiles';

Expand Down Expand Up @@ -49,23 +49,32 @@ export class FeatureService {
let whereQuery = `feature_id = '${id}'`;

if (bbox) {
whereQuery += `AND st_intersects(ST_MakeEnvelope(${nominatim2bbox(
bbox,
)}, 4326), the_geom)`;
const { westBbox, eastBbox } = antimeridianBbox(nominatim2bbox(bbox));
whereQuery += `AND
(st_intersects(
st_intersection(st_makeenvelope(${eastBbox}, 4326),
ST_MakeEnvelope(0, -90, 180, 90, 4326)),
the_geom
) or st_intersects(
st_intersection(st_makeenvelope(${westBbox}, 4326),
ST_MakeEnvelope(-180, -90, 0, 90, 4326)),
the_geom
))`;
}
return whereQuery;
}

/**
* @todo get attributes from Entity, based on user selection
* @todo simplification level based on zoom level
*/
public findTile(
tileSpecification: TileSpecification,
bbox?: BBox,
): Promise<Buffer> {
const { z, x, y, id } = tileSpecification;
const attributes = 'feature_id, properties';
const table = `(select (st_dump(the_geom)).geom as the_geom, properties, feature_id from "${this.featuresRepository.metadata.tableName}")`;
const table = `(select ST_RemoveRepeatedPoints((st_dump(the_geom)).geom, 0.1) as the_geom, properties, feature_id from "${this.featuresRepository.metadata.tableName}")`;
Copy link
Member

Choose a reason for hiding this comment

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

🚀

const customQuery = this.buildFeaturesWhereQuery(id, bbox);
return this.tileService.getTile({
z,
Expand All @@ -76,4 +85,5 @@ export class FeatureService {
attributes,
});
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ export class PlanningAreaTilesService {
/**
* @todo this generation query is a bit...
*/
let whereQuery = `project_id = '${planningAreaId}'`;
const whereQuery = `project_id = '${planningAreaId}'`;

return whereQuery;
}
Expand Down
Loading