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

Finalise shareKeys #5055

Merged
merged 32 commits into from
Dec 16, 2020
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
1b09433
add v7 catalog support through v7initializationUrls
nf-s Dec 2, 2020
cd6cd4c
add test
nf-s Dec 2, 2020
1b2f479
Merge remote-tracking branch 'origin/next' into v7-catalog-support-clean
nf-s Dec 9, 2020
aac476d
add v7initUrls docs + fix wrong v7 autoID
nf-s Dec 9, 2020
9bbcc1c
add magda record terria aspect sharekeys to root group
nf-s Dec 9, 2020
7c1a9f9
fix bad typing
nf-s Dec 9, 2020
fc7b9c1
add dodgy GroupMixin shareKeys
nf-s Dec 9, 2020
4b8c333
add dynamic group sharekeys ADR
nf-s Dec 10, 2020
4c611f8
add 0006 adr decision
nf-s Dec 10, 2020
9b115c2
adr type
nf-s Dec 10, 2020
87c23f1
change WMS-group member ids to match v7
nf-s Dec 10, 2020
bdd332a
add group shareKeys propagation
nf-s Dec 10, 2020
77a25b6
fix tests
nf-s Dec 10, 2020
1be250c
fix test
nf-s Dec 11, 2020
5d13c2b
add WMS group `skareKeys` test
nf-s Dec 11, 2020
7839d85
bump catalog converter versoin
nf-s Dec 11, 2020
cd079bb
fix legend image scaling
nf-s Dec 14, 2020
717152a
fix test
nf-s Dec 15, 2020
66905da
Merge remote-tracking branch 'origin/next' into v7-catalog-support-clean
nf-s Dec 15, 2020
5d072b1
udpate changes
nf-s Dec 15, 2020
05369a9
comment
nf-s Dec 15, 2020
f4804ef
changes
nf-s Dec 15, 2020
a3eb9a5
move catalog-share conversion from server to terriajs
nf-s Dec 15, 2020
9a5501b
remove makeModelsMagdaCompatible
nf-s Dec 16, 2020
9059ccd
Update lib/ModelMixins/GroupMixin.ts
nf-s Dec 16, 2020
e2d552a
Update lib/Models/Terria.ts
nf-s Dec 16, 2020
ca117c8
Update lib/ReactViews/Notification/shareConvertNotification.tsx
nf-s Dec 16, 2020
2b61cf8
fix geoserver legend font colour
nf-s Dec 16, 2020
f4ebdcd
refactor initSource.optoins
nf-s Dec 16, 2020
6f129b8
fix shareConvertNotification
nf-s Dec 16, 2020
81fb978
changes
nf-s Dec 16, 2020
c5e9fc3
remove unused vars
nf-s Dec 16, 2020
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
9 changes: 4 additions & 5 deletions architecture/0005-root-group-ids-and-shareKeys.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ Version: 3

## Status

In discussion
Accepted

## Context

Expand All @@ -21,10 +21,9 @@ In discussion

Most maps (v7 and v8) use autogenerated `id` for catalog members - these are used if catalog members don't have an `id` explicitly defined.

The root group `id` is very important for autoIDs. By default v8 uses `"/"` and v7 uses `"Root Group"`

The root group `id` is very important for autoIDs. By default v8 uses `"/"` and v7 uses `"/Root Group"`

- v7 ID has format `/Root Group/$someContainerId/$someLowerContainerId/$catalogName`
- v7 ID has format `Root Group/$someContainerId/$someLowerContainerId/$catalogName`
- v8 ID has format `//$someContainerId/$someLowerContainerId/$catalogName`

Using autoIDs solidifies catalog - you can't move items around without changing IDs, which will break share links.
Expand Down Expand Up @@ -197,7 +196,7 @@ This also means that if we chain transformations - for example:

We will have 3 `shareKeys` to maintain:

- v7 autoIDs: `/Root Group/$someContainerId/$someLowerContainerId/$catalogName`
- v7 autoIDs: `Root Group/$someContainerId/$someLowerContainerId/$catalogName`
- Old magda JSON based autoIDs: `$magda-config-record-id/$someContainerId/$someLowerContainerId/$catalogName`

Also, if we ever change the `$magda-config-record-id` - this will result in a whole new set of IDs that need to be added to `shareKeys` (for Magda JSON-based maps).
Expand Down
121 changes: 121 additions & 0 deletions architecture/0006-dynamic-groups-and-shareKeys.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
# 6. Dynamic groups and `shareKeys` / share link compatibility

Date: 2020-12-10
Version: 1

## Status

Accepted

## Context

See [architecture\0005-root-group-ids-and-shareKeys.md](architecture\0005-root-group-ids-and-shareKeys.md) for background.

### Glossary

- `wms-group` is `WebMapServiceCatalogGroup` - it loads `GetCapabilities` request to create `WebMapServiceCatalogItem`...
- `wms` is `WebMapServiceCatalogItem`

### How to handle loadable (dynamic) groups with ids - for example `wms-group`

#### Using autoIDs

autoIDs are generated hierarchically - using catalog member `name`.

- v7 autoID has format `Root Group/$someContainerId/$someLowerContainerId/$catalogName`
- v8 autoID has format `//$someContainerId/$someLowerContainerId/$catalogName`

For example - a `wms-group` will have:

- v7 autoID: `Root Group/$wmsGroup.name`
- v8 autoID: `//$wmsGroup.name`

and when it is loaded - `wms` items will have:

- v7 autoID: `Root Group/$wmsGroup.name/$wmsItem.name`
- v8 autoID: `//$wmsGroup.name/$wmsItem.name`

#### Missing `shareKeys`

Even if the `wms-group` has `shareKeys`, the `wms` item ID will be incorrect for a v7 share link

- wms-group v7 autoID will be `Root Group/$wmsGroup.name`
- which will be in `shareKeys` (this is added by `catalog-converter`) - so ID will be resolved correctly
- A wms-item v7 autoID will be `Root Group/$wmsGroup.name/$wmsItem.name`
- the item won't have any `shareKeys`, therefore the ID will **not** be resolved

#### Using manual `id` for `wms-group`

What happens when we set an explicit `id` for a `wms-group`?

- A `wms-group` used to have no `id`, therefore it had a v8 autoID `//$wmsGroup.name`
- Now the `wms-group` has `id`:`wms-group-random-id` (and `shareKeys=["//$wmsGroup.name"]`)
- All share links which refer to `//$wmsGroup.name` will automatically resolve to `wms-group-random-id`
- The `wms-group` is loaded and creates a bunch of `wms` items:
- Before `wms-group` `id` change, items had v8 autoID of form `//$wmsGroup.name/$wmsItem.name`
- After `id` change - `wms-group-random-id/$wmsItem.name`
- Items have **no `shareKeys`**
- Therefore, `wms` items in old share links **will not work!**

This is also relevant if moving from v8 JSON to v8 Magda catalog - or any other transformation which sets explicit `id`s for catalog members.

### Auto IDs for `CatalogGroups`

Different dynamic `CatalogGroups` may generate member autoIDs differently.

#### WMS-Group

For example - `wms-group` autoIDs for `wms` items are generated from the item `name`:

- in v8: `${layer.Name || layer.Title}`
- in v7:

```js
if (wmsGroup.titleField === "name") {
return layer.Name;
} else if (wmsGroup.titleField === "abstract") {
return layer.Abstract;
} else {
return layer.Title;
}
```

#### CKAN Resource

- in v8: `parentId + "/" + ckanDataset.id + "/" + ckanResource.id`

### Summary

How do we add `shareKeys` to items created by dynamic groups?

## Decisions

### Add `shareKeys` to group items on load

- Go through each share key in `group.shareKeys` and create new `member.shareKeys`:
- Look at current `member.uniqueId`
- Replace instances of `group.uniqueID` in `member.uniqueId` with `shareKey`
- For example:
- `group.uniqueId = "some-group-id"`
- `member.uniqueId = "some-group-id/some-member-id"`
- `group.shareKeys = ["old-group-id"]`
- So we want to create `member.shareKeys = ["old-group-id/some-member-id"]`

To do this, we need a reverse map of `id` -> `shareKeys`.

### Revert v8 WMS-group member ids to match v7

This means we don't need to add manual `shareKeys` to WMS-groups.

## Consequences

This does not solve for the different methods of generating autoIDs across v7 and v8.

### Known incompatible groups

- CKAN Group

If we have compatibility issues with certain `CatalogGroup`s, we can either:

- Change id generation in v8 to match v7
- Add manual `shareKey` generation to each `CatalogGroup`
4 changes: 4 additions & 0 deletions doc/customizing/client-side-config.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ If a string ends with `.json`, it is assumed to be a complete relative or absolu

If the string does not end with `.json`, such as `"foo"`, it refers to an init file on the same web server at `init/foo.json`. In a TerriaMap directory on your computer, it can be found at `wwwroot/init/foo.json`.

### v7intializationUrls

It is also possible to add version 7 init files — these will be converted on-the-fly in `terriajs` when a map is loaded. See [`catalog-converter`](https://github.com/TerriaJS/catalog-converter) repo for more information.

## parameters

Specifies various options for configuring TerriaJS:
Expand Down
11 changes: 6 additions & 5 deletions lib/Core/addedByUser.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,17 @@
import { BaseModel } from "../Models/Model";
import i18next from "i18next";
import isDefined from "./isDefined";
export const USER_ADDED_CATEGORY_ID = "__User-Added_Data__";

export default function addedByUser(
catalogMember: BaseModel,
catalogMember: BaseModel | undefined,
options: {
depth: number;
} = {
depth: 0
}
): boolean {
if (!isDefined(catalogMember)) return false;
const depth = options.depth;
if (depth > 100) {
console.error(
Expand All @@ -26,10 +28,9 @@ export default function addedByUser(
return sourceReference.knownContainerUniqueIds.some(containerId => {
return (
containerId === USER_ADDED_CATEGORY_ID ||
addedByUser(
<BaseModel>catalogMember.terria.getModelById(BaseModel, containerId),
{ depth: depth + 1 }
)
addedByUser(catalogMember.terria.getModelById(BaseModel, containerId), {
depth: depth + 1
})
Comment on lines -29 to +33
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed bad <BaseModel> typing here

);
});
}
46 changes: 39 additions & 7 deletions lib/ModelMixins/GroupMixin.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
import { action, computed, observable, runInAction } from "mobx";
import DeveloperError from "terriajs-cesium/Source/Core/DeveloperError";
import { action, computed } from "mobx";
import clone from "terriajs-cesium/Source/Core/clone";
import DeveloperError from "terriajs-cesium/Source/Core/DeveloperError";
import AsyncLoader from "../Core/AsyncLoader";
import Constructor from "../Core/Constructor";
import filterOutUndefined from "../Core/filterOutUndefined";
import isDefined from "../Core/isDefined";
import Group from "../Models/Group";
import Model, { BaseModel } from "../Models/Model";
import GroupTraits from "../Traits/GroupTraits";
import ModelReference from "../Traits/ModelReference";
import AsyncLoader from "../Core/AsyncLoader";
import Group from "../Models/Group";
import CatalogMemberMixin from "./CatalogMemberMixin";

function GroupMixin<T extends Constructor<Model<GroupTraits>>>(Base: T) {
abstract class GroupMixin extends Base implements Group {
Expand Down Expand Up @@ -65,9 +66,8 @@ function GroupMixin<T extends Constructor<Model<GroupTraits>>>(Base: T) {
*/
loadMembers(): Promise<void> {
return this._memberLoader.load().finally(() => {
if (this.uniqueId) {
this.refreshKnownContainerUniqueIds(this.uniqueId);
}
this.refreshKnownContainerUniqueIds(this.uniqueId);
this.addShareKeysToMembers();
});
}

Expand All @@ -81,6 +81,38 @@ function GroupMixin<T extends Constructor<Model<GroupTraits>>>(Base: T) {
});
}

@action
addShareKeysToMembers(): void {
const groupId = this.uniqueId;
if (!groupId) return;

// Get shareKeys for this Group
const shareKeys = this.terria.modelIdShareKeysMap.get(groupId);
if (!shareKeys || shareKeys.length === 0) return;

/**
* Go through each shareKey and create new shareKeys for members
* - Look at current member.uniqueId
* - Replace instances of group.uniqueID in member.uniqueId with shareKey
* For example:
* - group.uniqueId = 'some-group-id'
* - member.uniqueId = 'some-group-id/some-member-id'
* - group.shareKeys = 'old-group-id'
* - So we want to create member.shareKeys = ["old-group-id/some-member-id"]
*/

this.memberModels.forEach((model: BaseModel) => {
// Only add shareKey if model.uniqueId is an autoID (i.e. contains groupId)
if (!model.uniqueId || !model.uniqueId.includes(groupId)) return;
shareKeys.forEach(groupShareKey =>
this.terria.addShareKey(
model.uniqueId!,
model.uniqueId!.replace(groupId, groupShareKey)
)
);
});
}

@action
add(stratumId: string, member: BaseModel) {
if (member.uniqueId === undefined) {
Expand Down
13 changes: 12 additions & 1 deletion lib/Models/InitSource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,13 @@ interface InitData {
data: JsonObject;
}

type InitDataPromise = Promise<InitData>;

interface InitOptions {
options: InitSource[];
}

type InitSource = InitUrl | InitData | InitOptions;
type InitSource = InitUrl | InitData | InitOptions | InitDataPromise;

export function isInitUrl(initSource: InitSource): initSource is InitUrl {
return "initUrl" in initSource;
Expand All @@ -25,6 +27,15 @@ export function isInitData(initSource: InitSource): initSource is InitData {
return "data" in initSource;
}

export function isInitDataPromise(
initSource: InitSource
): initSource is InitDataPromise {
return (
initSource &&
Object.prototype.toString.call(initSource) === "[object Promise]"
);
}

Comment on lines +30 to +38
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used for v7-v8 catalog converting

export function isInitOptions(
initSource: InitSource
): initSource is InitOptions {
Expand Down
Loading