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

Url proxy endpoint featuring OpenAPI #141

Conversation

kasperg
Copy link
Contributor

@kasperg kasperg commented Oct 5, 2022

Link to issue

https://reload.atlassian.net/browse/DDFSOEG-245

Description

The purpose of this PR is twofold:

  1. We need to expose an endpoint which supports transforming an "unproxied" url to a url which has been modified to support proxies used by a library agency for access control based on the configuration provided by the library. This endpoint can be used by the React components which render data containing unproxied urls. This is handled by the UrlProxyResource class.
  2. We want a standardized approach to exposing functionality and data from Drupal to external parties. We have looked into our options and argue that we should use Drupals REST and OpenAPI modules.

This PR does the following:

  • Add and patch the required modules to provide a valid OpenAPI specification in composer.json
  • Replace the url proxy controller and routing with a resource and configuration and update tests accordingly
  • Add an initial OpenAPI 2.0 specification in openapi.json
  • Add a GitHub Actions job with tasks to ensure that our openapi.json specification stays valid and does not drift
  • Add documentation describing reasoning and future development process for API endpoints
  • Some other boy scout changes found when working with the codebase

Checklist

  • My complies with our coding guidelines.
  • My code passes our static analysis suite. If not then I have added a comment explaining why this change should be exempt from the code standards and process.
  • My code passes our continuous integration process. If not then I have added a comment explaining why this change should be exempt from the code standards and process.

spaceo and others added 30 commits October 3, 2022 17:11
and modified dpl_url_proxy setup/test
By using a ParamConverter and url encoding json.
Following data works after /dpl-url-proxy/ :
{
  "scheme": "http",
  "host": "john.com",
  "port": "",
  "path": "",
  "query": "kalle=1",
  "fragment": ""
}

It results in following url:
%7B%0A%20%20%22scheme%22%3A%20%22http%22%2C%0A%20%20%22host%22%3A%20%22john.com%22%2C%0A%20%20%22port%22%3A%20%22%22%2C%0A%20%20%22path%22%3A%20%22%22%2C%0A%20%20%22query%22%3A%20%22kalle%3D1%22%2C%0A%20%20%22fragment%22%3A%20%22%22%0A%7D%0A

PhpUnitTest breaks now though because:
* The endpoint urls in tests needs to be updated with new url structure
* The UrlParamConverter does not seem to be loaded in test context
It is nice to have a sensible order.
Otherwise we will not be able to use it with other programmatic tools.
This is necessary for us to be able to specify the API at a level where
it is usable for us in client generation.
This way we avoid a lot of complexity caused by wonky encoding required
by the url param converter.

Patches to the OpenAPI REST module means that we will be able to
specify query parameter structure anyway.
This is now handled by our rest resource instead.
There is no need for a double nested objects. An object with an url key
should do fine.

Specifying this is now supported through our patches to the OpenAPI REST
module.
We need to explicitly specify the XDEBUG mode for this to work.

- coverage: This gets us coverage needed for output
- debug: This enables step debugging which can be helpful when writing
         unit tests
There is no need to keep it a secret and having it available is useful
for automations.
This allows us to use the API with other tools without having a running
site.

Also it allows us to detect changes to the specification which may occur
unintentionally as it partly autogenerated based on configuration.

Our specification it somewhat handheld so ensuring that it is valid is
also helpful.

Node tools swagger-cli and jsome helps us with validation and 
formatting.
These are individual per site and should not be overwritten with future
updates. By keeping the configuration a part of the ignored settings
we ensure this.
This is in line with other modules
This allows us to return metadata about this response, links etc.

Update the specification accordingly
They should not be enabled by default on production sites.
We have decided on using OpenAPI and REST for API development and
specification. Thus we no longer need modules related to JSON:API.

OpenAPI UI Redoc is not necessary for us to work with or expose our API
so remove it entirely. This reduces our code surface.
This is not needed for JavaScript code - only JSON.
The default 4x throttle is not relevant when running Lighthouse in
containers or in GitHub Actions.
The cpu multiplier value must be wrapped in a throttling entry.
@github-actions github-actions bot temporarily deployed to pr-141 October 5, 2022 13:22 Inactive
@kasperg kasperg marked this pull request as ready for review October 5, 2022 13:26
Use query args as cache context. It is more specific to what we are
doing.

Also remember to use snake_case instead of camelCase for variables.
@github-actions github-actions bot temporarily deployed to pr-141 October 6, 2022 06:38 Inactive
config/sync/user.role.anonymous.yml Outdated Show resolved Hide resolved
config/sync/user.role.authenticated.yml Outdated Show resolved Hide resolved
@github-actions github-actions bot temporarily deployed to pr-141 October 6, 2022 13:48 Inactive
@kasperg kasperg merged commit 4892d46 into release/4 Oct 6, 2022
@kasperg kasperg deleted the opsaet-openapi-understottelse-og-ensretning-af-rest-api-opsaetning-DDFSOEG-245 branch October 6, 2022 13:53
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