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

Jump to bottom

add the csv distribution when dataset is a proxied csv #20

Merged
merged 2 commits into from
Feb 22, 2022
Merged

add the csv distribution when dataset is a proxied csv #20

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
994aea5
add the csv distribution when dataset is a proxied csv
sonofflynn89 Feb 17, 2022
24d98d9
changelog
sonofflynn89 Feb 17, 2022
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
40 changes: 20 additions & 20 deletions example-app/package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

54 changes: 39 additions & 15 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 9 additions & 5 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
"url": "git@github.com:koopjs/koop-output-dcat-us-11.git"
},
"devDependencies": {
"@esri/arcgis-rest-feature-layer": "^3.2.1",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@drspacemanphd this is what I was talking about. This package is declared as a dependency in the example app, but when I run npm run dev the script chokes on a typescript error, saying that an interface from this package is not defined. Adding it as a dev and a peer dependency solves the issue and shouldn't affect consumers.

"@types/adlib": "^3.0.1",
"@types/config": "0.0.39",
"@types/express": "^4.17.13",
Expand Down Expand Up @@ -57,15 +58,18 @@
"@esri/arcgis-rest-auth": "^3.2.1",
"@esri/arcgis-rest-portal": "^3.2.1",
"@esri/arcgis-rest-request": "^3.2.1",
"@esri/hub-common": "^9.8.0",
"@esri/hub-initiatives": "^9.8.0",
"@esri/hub-search": "^9.8.0",
"@esri/hub-sites": "^9.8.0",
"@esri/hub-teams": "^9.8.0",
"@esri/hub-common": "^9.17.1",
"@esri/hub-initiatives": "^9.17.1",
"@esri/hub-search": "^9.17.1",
"@esri/hub-sites": "^9.17.1",
"@esri/hub-teams": "^9.17.1",
"config": "^3.3.6",
"lodash": "^4.17.21",
"tslib": "~2.3.0"
},
"peerDependencies": {
"@esri/arcgis-rest-feature-layer": "^3.2.1"
},
"volta": {
"node": "14.16.1"
}
Expand Down
37 changes: 37 additions & 0 deletions src/dcat-us/_generate-distributions.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,43 @@ describe('_generateDistributions', () => {
expect(distributions).toEqual(expected);
})

it('adds default and csv distributions if dataset is a proxied csv', () => {
const dataset = {
id: 'foo',
access: 'public',
slug: 'nissan::skyline-gtr',
size: 1,
type: 'CSV'
};

const expected = [{
'@type': 'dcat:Distribution',
title: 'ArcGIS Hub Dataset',
format: 'Web Page',
mediaType: 'text/html',
accessURL: 'https://my-site.hub.arcgis.com/datasets/nissan::skyline-gtr'
},
{
'@type': 'dcat:Distribution',
title: 'ArcGIS GeoService',
format: 'ArcGIS GeoServices REST API',
mediaType: 'application/json',
},
{
'@type': 'dcat:Distribution',
title: 'CSV',
format: 'CSV',
mediaType: 'text/csv',
accessURL: 'https://my-site.hub.arcgis.com/datasets/nissan::skyline-gtr.csv'
}
]

const distributions = _generateDistributions(dataset, getLandingPage(dataset.slug));

expect(distributions.length).toBe(3)
expect(distributions).toEqual(expected);
});

it('adds default distributions if dataset is a layer', () => {
const dataset = {
id: 'foo_0', // layer id
Expand Down
15 changes: 15 additions & 0 deletions src/dcat-us/_generate-distributions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { DatasetResource, datasetToItem, getProxyUrl, IHubRequestOptions } from '@esri/hub-common';
import * as _ from 'lodash';

const WFS_SERVER = 'WFSServer';
Expand All @@ -19,6 +20,10 @@ export function _generateDistributions (dataset: any, landingPage: string) {
getEsriGeoServiceDistribution
];

if (isProxiedCSV(dataset)) {
distributionFns.push(getCSVDistribution);
}

if (isLayer(dataset)) {
distributionFns.push(getGeoJSONDistribution);
distributionFns.push(getCSVDistribution);
Expand Down Expand Up @@ -53,6 +58,16 @@ function isLayer (dataset: any) {
return /_/.test(dataset.id);
}

function isProxiedCSV(dataset: any) {
const item = datasetToItem({
id: dataset.id,
attributes: dataset
} as DatasetResource);
const requestOptions: IHubRequestOptions = { isPortal: false };

return !!getProxyUrl(item, requestOptions);
Comment on lines +62 to +68
Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking out loud...

This set of conversions is clever, but squishy. We're basically taking a V3 API response, making it look sorta like a dataset, then making that sorta look like an item, then throwing it into the getProxyUrl method which expects a full item.

We can do this because we happen to know about the internals of getProxyUrl enough to know that this will work.

That said, if it works—great. A regression here would be low-impact and you have a test to validate it anyways.

One question though—why not just use isProxiedCSV instead of getProxyUrl?

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay—I see now that isProxiedCSV isn't part of the Hub.js public API. I'm not sure why—but you probably had a reason.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@andrewctate thanks for the review!

Okay—I see now that isProxiedCSV isn't part of the Hub.js public API. I'm not sure why—but you probably had a reason.

That was my original plan, but Tom is really trying to minimize hub.js' public footprint. I discussed it with him, but ultimately he asked for us to do it this way 🤷‍♂️

This set of conversions is clever, but squishy.

I know it's not ideal. Tom is doing a massive refactor for getting IHubContents that we'll probably use in the near future. The result of that refactor will already have the proxied csv stuff baked in 🙌

}

// HUBJS CANDIDATE
function getDownloadLinkFn (landingPage: string, dataset: any) {
const spatialReference = _.get(dataset, 'server.spatialReference');
Expand Down
4 changes: 4 additions & 0 deletions src/dcat-us/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,8 @@ describe('generating DCAT-US 1.1 feed', () => {
'id',
'type',
'slug',
'access',
'size',
'licenseInfo',
'structuredLicense',
'layer.geometryType',
Expand Down Expand Up @@ -186,6 +188,8 @@ describe('generating DCAT-US 1.1 feed', () => {
'id',
'type',
'slug',
'access',
'size',
'licenseInfo',
'structuredLicense',
'layer.geometryType',
Expand Down
2 changes: 2 additions & 0 deletions src/dcat-us/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ export function getDataStreamDcatUs11(siteUrl: string, siteModel: IModel, dcatCu
'id', // used for the dataset landing page URL
'type', // used for the dataset landing page URL
'slug', // used for the dataset landing page URL
'access', // used for detecting proxied csv's
'size', // used for detecting proxied csv's
'licenseInfo', // required for license resolution
'structuredLicense', // required for license resolution
...DISTRIBUTION_DEPENDENCIES,
Expand Down
2 changes: 1 addition & 1 deletion src/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ describe('Output Plugin', () => {
},
options: {
portal: 'https://www.arcgis.com',
fields: "id,type,slug,licenseInfo,structuredLicense,layer,server,metadata,url,name,description,tags,created,modified,source,owner,orgContactEmail,extent" },
fields: "id,type,slug,access,size,licenseInfo,structuredLicense,layer,server,metadata,url,name,description,tags,created,modified,source,owner,orgContactEmail,extent" },
});
});

Expand Down