Skip to content

Comments

delete /api/admin/test but add some TODOs #11760#44

Merged
qqmyers merged 1 commit intoGlobalDataverseCommunityConsortium:SPA-_api_to_getToolLaunchUrlfrom
IQSS:11760-rm-api-admin-test
Sep 3, 2025
Merged

delete /api/admin/test but add some TODOs #11760#44
qqmyers merged 1 commit intoGlobalDataverseCommunityConsortium:SPA-_api_to_getToolLaunchUrlfrom
IQSS:11760-rm-api-admin-test

Conversation

@pdurbin
Copy link
Member

@pdurbin pdurbin commented Sep 2, 2025

Enough functionality has been added in IQSS#11760 that it should be safe to delete all of /api/admin/test (TestApi.java) because it was only testing external tools.

However, if we incorporate this change (deleting that class) into that PR we should do the following:

  • fix the backend to not return a toolUrl when it shouldn't (see testFileLevelToolWithAuxFileReq)
  • consider how we aren't exercising findDatasetToolsByType and findFileToolsByType anymore. They are used by JSF.
  • consider how we don't return the following fields when getting the toolUrl: scope, types, contentType
  • remove commented code that's just there for review

This is the tool added with testFileLevelToolWithAuxFileReq:

curl --location 'http://localhost:8080/api/admin/externalTools' \
--header 'Content-Type: application/json' \
--data '{
  "displayName": "HDF5 Tool",
  "description": "Operates on HDF5 files",
  "types": [
    "preview"
  ],
  "scope": "file",
  "contentType": "application/x-hdf5",
  "toolUrl": "/dataexplore/dataverse-previewers/previewers/v1.3/TextPreview.html",
  "toolParameters": {
    "queryParameters": [
      {
        "fileid": "{fileId}"
      },
      {
        "siteUrl": "{siteUrl}"
      },
      {
        "key": "{apiToken}"
      }
    ]
  },
  "requirements": {
    "auxFilesExist": [
      {
        "formatTag": "NcML",
        "formatVersion": "0.1"
      }
    ]
  }
}'

In JSF, the tool is correctly not shown for the "false" file (random bytes with an extension of ".hdf5"):

PR11760

But via the new API, a tool is offered:

% curl -s -X POST http://localhost:8080/api/files/59/externalTool/142/toolUrl | jq .
{
  "status": "OK",
  "data": {
    "toolUrl": "/dataexplore/dataverse-previewers/previewers/v1.3/TextPreview.html?fileid=59&siteUrl=http://localhost:8080",
    "displayName": "HDF5 Tool",
    "fileId": 59,
    "preview": false
  }
}

Instead, the API should return something like "auxFilesExist requirement not met". From what I can tell, externalToolService.meetsRequirements() isn't called by the new API. This method was added in 9edaf59.

Enough functionality has been added in #11760 that it
should be save to delete all of /api/admin/test (TestApi.java)
because it was only testing external tools.

However, if we incorporate this change into that PR we should
do the following:

- fix the backend to not return a toolUrl when it shouldn't
(see testFileLevelToolWithAuxFileReq)
- consider how we aren't exercising findDatasetToolsByType
and findFileToolsByType anymore. They are used by JSF.
- consider how we don't return the following fields when
getting the toolUrl: scope, types, contentType
- remove commented code that's just there for review
@coveralls
Copy link

Pull Request Test Coverage Report for Build #1251

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 23.342%

Totals Coverage Status
Change from base Build #1246: 0.02%
Covered Lines: 21134
Relevant Lines: 90542

💛 - Coveralls

@qqmyers qqmyers merged commit 4c10b8a into GlobalDataverseCommunityConsortium:SPA-_api_to_getToolLaunchUrl Sep 3, 2025
8 checks passed
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