Converge with openspending conductor API

[subhankarb]

There were total 3 issues to make dpr-api converge with open-spending API.

• 248

• 250

• 251

Making pull request for three separately was a problem as all three issues were closely related to each other . That is why raising the pull request for all three issues at a time. Below are the commits for each issues:

• 248 is resolved by https://github.com/subhankarb/dpr-api/commit/136899e43a8d7265100388083c9a2cc55e44344d

• 250 is resolved by https://github.com/subhankarb/dpr-api/commit/f356a6b6c7f5067734f3d52a8ecc2c007f2b7eb6

• 251 is resolved by https://github.com/subhankarb/dpr-api/commit/9ae8201c3dbb92865d0101dc547156c53635ac45

[rufuspollock]

Commit messages

First commit message is:

[l] Added API /api/bitstore/authorize_upl -refs #248

As i understand the commit this is simply false! You are actually adding /api/datastore/authorise (?).

For a large commit there is a minimal summary. As i note in the review this makes it very hard to understand what is going on in the change.

Second commit:

Modified bitstore upload finalize API - refs #251

Suggest could improve to:

[api]: Refactored finalize API for package upload to simple `/package/upload` - refs #251.

Part of our refactor to get more compatibility with the OpenSpending data package API.

Other comments

• This is largely a refactor / modify change. As such, I was not sure I could see where we were removing the old APIs and their tests?
• General point (unrelated this commit): Why two separate files: app/auth/annotations.py and app/utils/auth_helper.py?

[subhankarb]

We want to converge API of both dpr-api and os-conductor. The epic frictionlessdata/dpr-api#249 addressed all the changes we wanted to make so that dpmpy works with both the APIs.

In epic frictionlessdata/dpr-api#249 we mentioned three issues that is related to dpr-api.

• 248 Create /api/bitstore/authorize endpoint. The Commit 136899e43a8d7265100388083c9a2cc55e44344d contains the changes.

  • Previously our url pattern was {api}/{publisher}/{package} so we always get publisher and package information. That is why writing annotation was easy. But with {api}/bitstore/authorize we need to get publisher and package information from request body. So i am separating the is_authorize logic from annotation and put it in separate method, so that we can call it from controller.
  • Added model FileData. In older implementation we had only one s3 url call. But now we have list of files and have to generate a structured json response. That is why separating json building logic from controller is better. It is more testable.
  • Added a method to check datapackage already exists for validation.
  • Added related tests.

• 250 Use 'Auth-Token' instead of 'Authentication' header. The commit f356a6b6c7f5067734f3d52a8ecc2c007f2b7eb6 contains the changes.

  • Add additional check for jwt token for header key 'Auth-Token' and remove bearer from token prefix.

• 251 Change upload finalization API. The commit 9ae8201c3dbb92865d0101dc547156c53635ac45 contains the changes.

  • Update {api}/{publisher}/{package}/finalize API to {api}/upload

This is largely a refactor / modify change. As such, I was not sure I could see where we were removing the old APIs and their tests?

• Remove GetS3SignedUrlTestCase. And updated older tests codes.

General point (unrelated this commit): Why two separate files: app/auth/annotations.py and app/utils/auth_helper.py?

• It can be merged. The we can change the app/auth/annotations.py to app/auth/auth_helper.py

[rufuspollock]

@subhankarb the summary is much better -- well done 👍 :1st_place_medal: .

Only thing missing was to have had a classic commit summary first line e.g.

[refactor][l]: Refactor to converge our API with openspending conductor API.

...