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

REST API serves configuration inefficiently #9056

Open
J4bbi opened this issue Sep 8, 2023 · 2 comments · May be fixed by DSpace/RestContract#240 or #9140
Open

REST API serves configuration inefficiently #9056

J4bbi opened this issue Sep 8, 2023 · 2 comments · May be fixed by DSpace/RestContract#240 or #9140
Assignees
Labels
bug interface: REST API v7+ REST API for v7 and later (dspace-server-webapp module) needs discussion Ticket or PR needs discussion before it can be moved forward.

Comments

@J4bbi
Copy link
Contributor

J4bbi commented Sep 8, 2023

Describe the bug
I think the configuration REST endpoint is inefficient.

It was created for a single use case, back in 2020. To pass the Google Analytics key to the front end from the backend. By now there are 17 backend configuration variables exposed via the rest end point

You can only get one variable per request at the moment, the findAll method is not implemented. A separate call is needed for each config you want, this creates overhead work and uses resources.

The RSS component, which is embedded by the way in the Pagination component makes at least two requests for metadata properties every time it's loaded.

A request for the Google Analytics key is being made on every single load of a page because the front end does not know if GA is enabled and needs to ask the backend every single time (see #9014).

It's not surprising that the new frontend is resource intensive if it is making several requests per page load.

Example
https://sandbox.dspace.org/browse/dateissued, half a sec wasted on GA. There is also another request for a metadata property there, registration.verification.enabled taking 185 ms and 1.1 kb, bandwidth and resources

image

Solution
To deprecate the configuration REST endpoint as it will have no practical use.

Instead, repurpose the whitelisting functionality for the REST configuration endpoint to expose those properties in the root REST endpoint. /server/api.

The root REST endpoint already fetches config to display so conceptually the root endpoint already exposes configuration properties.

Another reason to use the root endpoint is that, to the best of my understanding, every page load requests that endpoint early in it's build stage. So there is no need for a separate request for config.

it already has:

{
  "dspaceUI" : "https://sandbox.dspace.org/",
  "dspaceName" : "DSpace Sandbox",
  "dspaceServer" : "https://sandbox.dspace.org/server",
  "dspaceVersion" : "DSpace 8.0-SNAPSHOT",
  "type" : "root",
}

why not add

{
  "dspaceUI" : "https://sandbox.dspace.org/",
  "dspaceName" : "DSpace Sandbox",
  "dspaceServer" : "https://sandbox.dspace.org/server",
  "dspaceVersion" : "DSpace 8.0-SNAPSHOT",
  "type" : "root",
  "configuration" : [
    {
       "name": "google.analytics.key",
       "values": [ 
           "UA-XXXXXX-X"
      ]
   }]
}

This would also resolve the issue of returning 404 or anything at all if a request is made for a non defined property.

Finally, this will increase efficiency by reducing the number of request per page load by anywhere from two to several, depending on the page.

@J4bbi J4bbi added bug interface: REST API v7+ REST API for v7 and later (dspace-server-webapp module) needs triage New issue needs triage and/or scheduling labels Sep 8, 2023
@tdonohue tdonohue added needs discussion Ticket or PR needs discussion before it can be moved forward. and removed needs triage New issue needs triage and/or scheduling labels Sep 8, 2023
@tdonohue
Copy link
Member

This was discussed in today's DevMtg. Generally consensus that we need to improve the manner in which we make backend configurations available to the frontend. All agree this is inefficient & this design doesn't scale well.

Option 1: Some discussion of @J4bbi 's proposal (see "Solution" section of description above) to potentially remove the /api/config/properties endpoint entirely, and move all configurations to the root endpoint /api (as some core configs like dspace.server.url and dspace.ui.url already are shared on root endpoint).

  • Benefit: This endpoint is already called/used frequently and has some configs embedded. Using it would be one less endpoint to call.
  • Downside: this root endpoint /api is called by the UI to simply determine if the backend is responding. That means its response couldn't be cached (like other endpoints).
    • This also would be a major change to REST Contract, and would break backwards compatibility.

Option 2: Another option is to update the existing endpoint to return all exposed configurations when calling GET /api/config/properties. (Currently this returns a 405 per the contract)

  • Benefit: Can still load all the configs at once instead of the current one-by-one approach.
    • While this changes the REST Contract minimally, it's "backwards compatible". The one-by-one approach will still work, but this allows for better performance if you load them all at once. @tdonohue feels this might be allowed in a bug-fix release, as it is arguably fixing a performance bug, and it doesn't break backwards compatibility.
  • Downside: None, except that we see the potential benefits to "Option 1" (see above) in possibly removing the endpoint. But, that would require much larger code changes.

@abollini, @benbosman and @KevinVdV : I want to ping all three of you regarding this discussion as you / your teams were all involved with the original design of the /api/config/properties endpoint in DSpace/RestContract#119 and #2762 If you have strong opinions on this possible improvement, I'd love to hear them. I'd especially like to hear if "Option 2" above seems reasonable to you as I'm seriously considering whether we should do this in a bug fix release to fix the inefficient behavior we are seeing (which can impact performance)

@tdonohue
Copy link
Member

This was discussed again in today's DevMtg.

During that meeting, @abollini pointed out that this might be better resolved via a new /search endpoint like the following:

  • /api/config/properties/search/exposed -> return a list of all properties listed in "rest.properties.exposed" in backend configs.

This would be a "canned search" (similar to /communities/search/top) which doesn't allow for any additional parameters. The behavior would be identical to "Option 2" in my prior comment.

The two main benefits to this search endpoint are as follows:

  • Completely backwards compatible & could be added easily to 7.6.x. Requires no changes to current endpoint behavior as this is a new endpoint.
  • Better describes the purpose of the endpoint, which is to return all "exposed" properties... and not all properties (obviously we are not sharing most configs via this endpoint). So, it's more self-explanatory.

@J4bbi J4bbi self-assigned this Sep 29, 2023
@J4bbi J4bbi linked a pull request Sep 30, 2023 that will close this issue
@J4bbi J4bbi linked a pull request Oct 23, 2023 that will close this issue
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug interface: REST API v7+ REST API for v7 and later (dspace-server-webapp module) needs discussion Ticket or PR needs discussion before it can be moved forward.
Projects
Status: 🏗 In Progress
2 participants