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

Cleanup API spec #432

Merged
merged 41 commits into from
Apr 29, 2019
Merged

Cleanup API spec #432

merged 41 commits into from
Apr 29, 2019

Conversation

matthewhanson
Copy link
Collaborator

Initial draft, still need to clean up and expand the documentation in README.

This PR:

  • consolidates documentation into README.md and search.md. There was a bunch of redundant or outdated information
  • Improve process and documentation on creating OpenAPI from existing definitions, fragments, extensions

@cholmes cholmes added this to the 0.7.0 milestone Apr 20, 2019
@cholmes
Copy link
Contributor

cholmes commented Apr 20, 2019

Hey @matthewhanson - just realized we have an approved PR for some API stuff - #415 I was about to merge, but then thought I should pause in case it might mess up this PR.

api-spec/README.md Outdated Show resolved Hide resolved
api-spec/extensions/README.md Outdated Show resolved Hide resolved
api-spec/extensions/transaction/README.md Outdated Show resolved Hide resolved
api-spec/api-spec.md Show resolved Hide resolved
@m-mohr
Copy link
Collaborator

m-mohr commented Apr 26, 2019

I added some comments and have some more thing I wasn't able to add review comments for:

  • The link to the implementation.md in api-spec/extensions/transaction/README.md is broken.
  • Add a prominent link from the main extension page to the API extensions.

api-spec/api-spec.md Outdated Show resolved Hide resolved
api-spec/api-spec.md Outdated Show resolved Hide resolved
api-spec/api-spec.md Outdated Show resolved Hide resolved
Cleaned up some of the intro Item/GeoJSON language

Changed WFS3 to WFS 3 - that's how they talk about it and it shows up better in google searches

Added a bit more about STAC + WFS 3
tweaked a few things, changed from WFS3 to WFS 3
Copy link
Contributor

@cholmes cholmes left a comment

Choose a reason for hiding this comment

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

Haven't got to fully review everything yet. And did a couple direct commits to the branch, I hope that was ok. Feel free to rollback if not. I held off on major things there.

I do think we should go ahead and merge this soon, and then we can do additional changes as PR's and discuss that way. Since overall this is a big step in the right direction, and most changes requested are minor.


## API Fragments
**Schemas:** The API is described by the OpenAPI specification documents in the *[openapi](openapi/)* folder.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to call this 'schemas', as they aren't really schemas in the sense of JSON or XML schemas. I think we can just say 'OpenAPI documents' or 'OpenAPI fragments', etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

From https://swagger.io/docs/specification/about/ it seems like maybe the best term is 'OpenAPI description'.

api-spec/api-spec.md Outdated Show resolved Hide resolved
api-spec/api-spec.md Outdated Show resolved Hide resolved
| /stac/search | Items | GeoJSON FeatureCollection of Items found |
```

The `stac/search` endpoint is similar to the `items` endpoint in WFS3 in that it accepts parameters for filtering, however it performs the filtering across all collections. The parameters accepted are the same as the Filter Parameters above, however the *[extensions](extensions/README.md)* also provide advanced querying parameters.
Copy link
Contributor

Choose a reason for hiding this comment

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

So I'd like to add a line here like: 'The stac endpoint should function as a complete Catalog representation of all the data contained in the STAC API - all Items should be linked in some way from that root through catalogs/collections, and as item links from those catalogs. But then I realized that sat-api isn't doing that yet.

I feel like we should have an endpoint that stac browser can point at and navigate through down to the items in the same way you can with a static catalog. Though perhaps it makes sense to call that endpoint /browse or something, and then it'd be easier to not make it required?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

STAC Browser can now browse APIs because it can @mojodna added the ability to resolve links with rel=items as well as rel=item.

I actually removed the existing paragraph about the API sub-catalogs and opened an issue for it:
https://github.com/radiantearth/stac-spec/issues/438

So in short, yes we should discuss how a browsable API should work and can add a better description of it, but we can make it another PR.

@matthewhanson
Copy link
Collaborator Author

I added some comments and have some more thing I wasn't able to add review comments for:

  • Add a prominent link from the main extension page to the API extensions.

@m-mohr Not sure what you mean here. The table of API extensions in extensions/README.md does link to each one. Or do you mean something else?

@matthewhanson
Copy link
Collaborator Author

@m-mohr @cholmes
Ok I resolved all these issues except for the Browsable API, which is a new issue #438 and a link from the
main extension page to API extensions.

@matthewhanson
Copy link
Collaborator Author

Oh, and also didn't update Open API schema to something else yet, but will update that everywhere when we agree what it should be called.
Rather than "openapi-schema" should it be "openapi-description"? OpenAPI Schema => OpenAPI Description ?

@m-mohr
Copy link
Collaborator

m-mohr commented Apr 28, 2019

I added some comments and have some more thing I wasn't able to add review comments for:

  • Add a prominent link from the main extension page to the API extensions.

@m-mohr Not sure what you mean here. The table of API extensions in extensions/README.md does link to each one. Or do you mean something else?

I meant the extension page with the content extensions (/extensions/README.md) should link to the API extensions (/api-spec/extensions/README.md).

Regarding the other issue, I would just call the folder "openapi". In tests I usually use OpenAPI document for the full file, and OpenAPI fragments for parts.

@matthewhanson
Copy link
Collaborator Author

Ok thanks, I added a couple sentences in the extensions/ folder differentiating between content and API extensions and added link.

Also removed "schema" from OpenAPI references.

That takes care of all the comments so far, other than the Browsable API, which is handles by #438

Copy link
Collaborator

@m-mohr m-mohr left a comment

Choose a reason for hiding this comment

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

Looks good now, great work. Added some minor changes (URL fixed + dependency update).

Will merge now so that I can base my changes on this PR.

@m-mohr m-mohr merged commit 14006c3 into dev Apr 29, 2019
@m-mohr m-mohr deleted the feature/cleanup_api branch April 29, 2019 08:12
m-mohr added a commit that referenced this pull request Apr 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants