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.

Sign up for GitHub

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

feat: require additional fields for historical imagery (require linz extension) #199

Merged
merged 6 commits into from
Dec 9, 2021
Merged

feat: require additional fields for historical imagery (require linz extension) #199

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c1736a4
feat: require additional fields for historical imagery
amfage Dec 9, 2021
3fa81f9
fix: linting
amfage Dec 9, 2021
b3a0eab
fix: added git tag example to readme
amfage Dec 9, 2021
4ddd6ee
Merge branch 'master' into feat/historical-imagery-new-requirements
amfage Dec 9, 2021
98a13f3
fix: fixed typo
amfage Dec 9, 2021
572c3d2
Merge branch 'feat/historical-imagery-new-requirements' of github.com…
amfage Dec 9, 2021
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
3 changes: 2 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ Verify changes by running `yarn lint && yarn test` before committing.
To create a release:

1. Run `yarn version --new-version <patch|minor|major>` which will create a change log commit and version tag.
2. Run `git push --atomic origin master TAG` which will trigger a GitHub release job. This will publish the new version to the "gh-pages" branch.
2. Run `git push --atomic origin master TAG` (e.g. `git push --atomic origin master v0.0.14`) which will trigger a GitHub release job.
This will publish the new version to the "gh-pages" branch.

## Adding a new extension

Expand Down
2 changes: 1 addition & 1 deletion extensions/aerial-photo/schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@
"type": "object",
"allOf": [
{
"required": ["type", "summaries"]
"required": ["type"]
},
{
"$ref": "#/definitions/fields"
Expand Down
12 changes: 12 additions & 0 deletions extensions/aerial-photo/tests/aerial_photo_collection.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,18 @@ o.spec('Aerial Photo Extension Collection', () => {
o(valid).equals(true)(JSON.stringify(validate.errors, null, 2));
});

o('Missing summaries section should pass validation', async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure to understand why this test. Did you add it because you've removed the summaries requirement in the schema?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, and I thought it would be good to check that it doesn't creep back in at some point. We want to keep these extensions as "light" as possible to make them easier for other people to use.

Copy link
Collaborator

@paulfouquet paulfouquet Dec 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

We want to keep these extensions as "light" as possible to make them easier for other people to use.

Totally agreed!

My question was focus on the unit test - as this tests that a non required field missing is not failing the validation and I did not understand why a test on this field when there are other non required fields that don't have a similar test. But I understand your point of view:

I thought it would be good to check that it doesn't creep back in at some point.

Copy link
Contributor

Choose a reason for hiding this comment

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

Basically it's a regression test.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What is you point of view on regression tests @l0b0 ? Should they stay there for good? Should we tag them for the code understanding later?

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting question. I'll try to answer with some personal "testing logic":

  1. The example file should contain examples of all the properties defined by the extension. Otherwise, what's the point?
  2. The first test shows that the file with all the properties is valid, to make sure we have a sane baseline.
  3. Subsequent tests show how this baseline can and can't be changed. By removing a mandatory property and asserting that the result fails validation we've tested that it is in fact mandatory. By removing an optional property and asserting that the result passes validation we've tested that it is in fact optional.
  4. It follows that removing the tests for mandatory or optional properties allows bugs to sneak in unnoticed.

So even though this is a regression test, it's not "just" a regression test. It verifies a useful feature of the schema which is not verified by other tests, so it should probably stay there for the foreseeable future.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great answer, thanks @l0b0 !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the interesting discussion @paulfouquet and @l0b0 , very useful.

// given
const example = JSON.parse(await fs.readFile(examplePath));
delete example.summaries;

// when
let valid = validate(example);

// then
o(valid).equals(true)(JSON.stringify(validate.errors, null, 2));
});

o("Summaries with no 'aerial-photo:run' property should fail validation", async () => {
// given
const example = JSON.parse(await fs.readFile(examplePath));
Expand Down
12 changes: 12 additions & 0 deletions extensions/camera/tests/camera_collection.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,4 +29,16 @@ o.spec('Camera Extension Collection', () => {
// then
o(valid).equals(true)(JSON.stringify(validate.errors, null, 2));
});

o('Missing summaries section should pass validation', async () => {
// given
const example = JSON.parse(await fs.readFile(examplePath));
delete example.summaries;

// when
let valid = validate(example);

// then
o(valid).equals(true)(JSON.stringify(validate.errors, null, 2));
});
});
2 changes: 1 addition & 1 deletion extensions/film/schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@
"type": "object",
"allOf": [
{
"required": ["type", "summaries"]
"required": ["type"]
},
{
"$ref": "#/definitions/fields"
Expand Down
12 changes: 12 additions & 0 deletions extensions/film/tests/film_collection.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,18 @@ o.spec('Film Extension Collection', () => {
o(valid).equals(true)(JSON.stringify(validate.errors, null, 2));
});

o('Missing summaries section should pass validation', async () => {
// given
const example = JSON.parse(await fs.readFile(examplePath));
delete example.summaries;

// when
let valid = validate(example);

// then
o(valid).equals(true)(JSON.stringify(validate.errors, null, 2));
});

o("Summaries with no 'film:id' property should fail validation", async () => {
// given
const example = JSON.parse(await fs.readFile(examplePath));
Expand Down
34 changes: 34 additions & 0 deletions extensions/historical-imagery/examples/collection.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,13 @@
"stac_version": "1.0.0",
"stac_extensions": [
"https://stac.linz.govt.nz/_STAC_VERSION_/historical-imagery/schema.json",
"https://stac.linz.govt.nz/_STAC_VERSION_/linz/schema.json",
"https://stac.linz.govt.nz/_STAC_VERSION_/quality/schema.json",
"https://stac-extensions.github.io/eo/v1.0.0/schema.json",
"https://stac-extensions.github.io/processing/v1.0.0/schema.json",
"https://stac-extensions.github.io/projection/v1.0.0/schema.json",
"https://stac-extensions.github.io/file/v2.0.0/schema.json",
"https://stac-extensions.github.io/version/v1.0.0/schema.json",
"https://stac.linz.govt.nz/_STAC_VERSION_/aerial-photo/schema.json",
"https://stac.linz.govt.nz/_STAC_VERSION_/camera/schema.json",
"https://stac.linz.govt.nz/_STAC_VERSION_/film/schema.json",
Expand All @@ -23,6 +27,32 @@
"interval": [["1947-03-28T12:00:00Z", "1952-04-22T12:00:00Z"]]
}
},
"linz:security_classification": "unclassified",
"linz:lifecycle": "completed",
"linz:history": "LINZ and its predecessors, Lands & Survey and Department of Survey and Land Information (DOSLI), commissioned aerial photography for the Crown between 1936 and 2008.\nOne of the predominant uses of the aerial photography at the time was the photogrammetric mapping of New Zealand, initially at 1inch to 1mile followed by the NZMS 260 and Topo50 map series at 1:50,000.\nThese photographs were scanned through the Crown Aerial Film Archive scanning project.",
"linz:providers": [
{
"name": "Toitū Te Whenua LINZ",
"description": "The New Zealand Government's lead agency for location and property information, Crown land and managing overseas investment.",
"roles": ["custodian"],
"url": "https://www.linz.govt.nz/about-linz/what-were-doing/projects/crown-aerial-film-archive-historical-imagery-scanning-project"
},
{
"name": "Manager Partnership Programmes",
"roles": ["manager"]
}
],
"linz:geospatial_type": "black and white image",
"linz:asset_summaries": {
"created": {
"minimum": "1999-01-01T00:00:00Z",
"maximum": "2021-12-09T00:04:09Z"
},
"updated": {
"minimum": "1999-01-02T00:00:00Z",
"maximum": "2021-12-09T00:04:09Z"
}
},
"providers": [
{
"name": "NZ Aerial Mapping",
Expand All @@ -36,6 +66,10 @@
"url": "https://www.linz.govt.nz/about-linz/what-were-doing/projects/crown-aerial-film-archive-historical-imagery-scanning-project"
}
],
"processing:software": {
"Topo Processor": "0.1.0"
},
"version": "1",
"summaries": {
"platform": ["Fixed-wing Aircraft"],
"instruments": ["EAGLE IV"],
Expand Down
15 changes: 12 additions & 3 deletions extensions/historical-imagery/examples/item.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,19 @@
{
"type": "Feature",
"stac_version": "1.0.0",
"stac_extensions": [
"https://stac.linz.govt.nz/_STAC_VERSION_/historical-imagery/schema.json",
"https://stac.linz.govt.nz/_STAC_VERSION_/linz/schema.json",
"https://stac-extensions.github.io/eo/v1.0.0/schema.json",
"https://stac-extensions.github.io/processing/v1.0.0/schema.json",
"https://stac-extensions.github.io/projection/v1.0.0/schema.json",
"https://stac-extensions.github.io/file/v2.0.0/schema.json",
"https://stac-extensions.github.io/version/v1.0.0/schema.json",
"https://stac.linz.govt.nz/_STAC_VERSION_/aerial-photo/schema.json",
"https://stac.linz.govt.nz/_STAC_VERSION_/camera/schema.json",
"https://stac.linz.govt.nz/_STAC_VERSION_/film/schema.json",
"https://stac.linz.govt.nz/_STAC_VERSION_/scanning/schema.json"
],
"type": "Feature",
"id": "215354",
"bbox": [170.54524, -45.80237, 170.56431, -45.81345],
"geometry": {
Expand Down Expand Up @@ -44,7 +47,11 @@
"film:physical_condition": "Film scratched",
"film:physical_size": "18cm x 23cm",
"scan:is_original": true,
"scan:scanned": "2018-09-30T11:00:00Z"
"scan:scanned": "2018-09-30T11:00:00Z",
"processing:software": {
"Topo Processor": "0.1.0"
},
"version": "1"
},
"links": [
{
Expand All @@ -68,7 +75,9 @@
"name": "gray",
"common_name": "pan"
}
]
],
"created": "2021-12-09T00:04:09Z",
"updated": "2021-12-09T00:04:09Z"
}
},
"collection": "01FJ0SE46TT5TGBHBVX64TMEPA"
Expand Down
9 changes: 7 additions & 2 deletions extensions/historical-imagery/schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
"properties": {
"type": "object",
"$comment": "Require fields here for Item Properties.",
"required": ["platform", "mission"]
"required": ["platform", "mission", "processing:software"]
},
"assets": {
"type": "object",
Expand Down Expand Up @@ -57,7 +57,7 @@
"allOf": [
{
"type": "object",
"required": ["type", "title", "providers", "summaries"]
"required": ["type", "title", "providers", "summaries", "processing:software"]
}
]
},
Expand All @@ -78,6 +78,11 @@
"const": "https://stac.linz.govt.nz/_STAC_VERSION_/historical-imagery/schema.json"
}
},
{
"contains": {
"const": "https://stac.linz.govt.nz/_STAC_VERSION_/linz/schema.json"
}
},
{
"contains": {
"const": "https://stac-extensions.github.io/eo/v1.0.0/schema.json"
Expand Down
34 changes: 34 additions & 0 deletions extensions/historical-imagery/tests/historical-imagery_collection.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,23 @@ o.spec('Historical Imagery Extension Collection', () => {
);
});

o("Example with missing 'processing:software' property should fail validation", async () => {
// given
const example = JSON.parse(await fs.readFile(examplePath));
delete example['processing:software'];

// when
let valid = validate(example);

// then
o(valid).equals(false);
o(
validate.errors.some(
(error) => error.instancePath === '' && error.message === "must have required property 'processing:software'",
),
).equals(true)(JSON.stringify(validate.errors));
});

o("Example without 'providers' should fail validation", async () => {
// given
const example = JSON.parse(await fs.readFile(examplePath));
Expand Down Expand Up @@ -227,4 +244,21 @@ o.spec('Historical Imagery Extension Collection', () => {
// then
o(valid).equals(true);
});

o("Example with no 'summaries' should fail validation", async () => {
// given
const example = JSON.parse(await fs.readFile(examplePath));
delete example.summaries;

// when
let valid = validate(example);

// then
o(valid).equals(false);
o(
validate.errors.some(
(error) => error.instancePath === '' && error.message === "must have required property 'summaries'",
),
).equals(true)(JSON.stringify(validate.errors));
});
});
17 changes: 17 additions & 0 deletions extensions/historical-imagery/tests/historical-imagery_item.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,4 +122,21 @@ o.spec('Historical Imagery Extension Item', () => {
),
).equals(true)(JSON.stringify(validate.errors));
});

o("Example without mandatory 'processing:software' property should fail validation", async () => {
// given
const example = JSON.parse(await fs.readFile(examplePath));
delete example.properties['processing:software'];
// when
let valid = validate(example);

// then
o(valid).equals(false);
o(
validate.errors.some(
(error) =>
error.instancePath === '/properties' && error.message === "must have required property 'processing:software'",
),
).equals(true)(JSON.stringify(validate.errors));
});
});
12 changes: 12 additions & 0 deletions extensions/scanning/tests/scanning_collection.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,4 +29,16 @@ o.spec('Scanning Extension Collection', () => {
// then
o(valid).equals(true)(JSON.stringify(validate.errors, null, 2));
});

o('Missing summaries section should pass validation', async () => {
// given
const example = JSON.parse(await fs.readFile(examplePath));
delete example.summaries;

// when
let valid = validate(example);

// then
o(valid).equals(true)(JSON.stringify(validate.errors, null, 2));
});
});