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

Define Endpoints for Search API #284

Merged
merged 16 commits into from
Nov 19, 2021

Conversation

MRichards99
Copy link
Collaborator

@MRichards99 MRichards99 commented Nov 3, 2021

This PR will close #257

Description

This PR defines endpoints which need to be implemented for the search API.

I've used the endpoint= argument for add_resource() to prevent naming collisions between endpoints. This was happening with endpoints like /datasets (DataGateway API endpoint) and /search_api/datasets even though they're actually different endpoints. Using endpoint= fixed this.

#256 isn't implemented yet so the search API endpoints are always created. Once the new config has been implemented, endpoint creation should respect the config and only create them when they are wanted by the user. I have also hardcoded the extension which should be changed to look up the relevant config value once #256 is closed.

To remember to resolve both TODOs, I have opened #283 to tackle these issues once #256 and #257 have been completed.

Testing Instructions

Run the API on this branch and ensure that you can send requests to each of the newly defined endpoints (e.g. /search_api/datasets) which return a 200 and a response body of null.

I was a little undecided on whether this needs a version bump or not. I've gone for feature because these changes do add in new endpoints, even if they are unimplemented. I would welcome an alternative view on this though, I'm not sure if a different (or even no) version bump is more appropriate.

  • Review code
  • Check GitHub Actions build
  • If icatdb Generator Script Consistency Test CI job fails, is this because of a deliberate change made to the script to change generated data (which isn't actually a problem) or is here an underlying issue with the changes made?
  • Review changes to test coverage
  • Does this change mean a new patch, minor or major version should be made? If so, does one of the commit messages feature fix:, feat: or BREAKING CHANGE: so a release is automatically made via GitHub Actions upon merge?

Agile Board Tracking

Connect to #257

- Matching DataGateway API's class structure, with relevant TODOs to add in code when endpoints are defined and implemented
- Since this repository will support DataGateway API and the Search API, there needs to be some changes in directory structure to split the different files up. Essentially this means adding `datagateway_api/` and `search_api/` in `common/` and `src/resources/`
- The imports will be fixed in a future commit
- Reverting back so the tests pass until I re-implement the config
- Directory structure due to implementation of search API
- These functions had the same function names as the ones defined for DataGateway API
- Collisions were occurring between `/datasets` between DataGateway API and the Search API despite the `/search_api` extension
@MRichards99 MRichards99 requested a review from VKTB November 3, 2021 14:29
@MRichards99 MRichards99 force-pushed the expands-search-api-structure branch from 0e35684 to b59abba Compare November 3, 2021 14:33
@codecov
Copy link

codecov bot commented Nov 3, 2021

Codecov Report

Merging #284 (896d3b8) into master (a2de545) will increase coverage by 1.14%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #284      +/-   ##
==========================================
+ Coverage   84.82%   85.97%   +1.14%     
==========================================
  Files          36       36              
  Lines        2584     2603      +19     
  Branches      207      208       +1     
==========================================
+ Hits         2192     2238      +46     
+ Misses        368      342      -26     
+ Partials       24       23       -1     
Impacted Files Coverage Δ
datagateway_api/src/api_start_utils.py 89.47% <100.00%> (+2.10%) ⬆️
...agateway_api/src/resources/search_api_endpoints.py 83.87% <100.00%> (+83.87%) ⬆️
...tagateway_api/src/swagger/apispec_flask_restful.py 54.38% <0.00%> (+1.75%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a2de545...896d3b8. Read the comment docs.

@MRichards99 MRichards99 linked an issue Nov 4, 2021 that may be closed by this pull request
2 tasks
VKTB
VKTB previously approved these changes Nov 5, 2021
Copy link
Contributor

@VKTB VKTB 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 and all the endpoints return null as expected. I feel as though a version bump is necessary though I am not sure which one should be used. Might be a good idea getting thoughts from others on this.

Base automatically changed from expands-search-api-structure to master November 19, 2021 12:04
@MRichards99 MRichards99 merged commit 9e137c6 into master Nov 19, 2021
@MRichards99 MRichards99 deleted the feature/search-api-endpoint-definition-#257 branch November 19, 2021 12:27
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.

Define Search API Endpoints
2 participants