[stevenwinship]

What this PR does / why we need it: External tools can't download files that require/have a guestbook response

Which issue(s) this PR closes:#12760

• Closes Previewers no longer working on files with required guestbooks, due to the API changes (since 6.10) #12760

Special notes for your reviewer:

Suggestions on how to test this: Create a dataset with multiple files to preview. Add a guestbook to the dataset and publish. Test previewing the files and download from the external tool. Test files with restricted access.
Download timeout comes from the external tool params. Try creating an external tool with a 1 minute timeout and make sure the signed url expires.

Does this PR introduce a user interface change? If mockups are available, please link/include them here:

Is there a release notes update needed for this change?: included

Additional documentation:

[coveralls]

Coverage is 25.666% — 12760-previewers-not-working-with-guestbook-responses into develop. No base build found for develop.

[github-actions]

Test Results

417 tests  ±0   403 ✅ ±0   24m 6s ⏱️ - 7m 48s
 59 suites ±0    14 💤 ±0 
 59 files   ±0     0 ❌ ±0 

Results for commit e148396. ± Comparison against base commit 3cbdeab.

♻️ This comment has been updated with latest results.

[landreev]

Could you please sync the branch up with develop.

[landreev]

@stevenwinship
It may be something I'm doing, but it's still not working for me.
I have an audio file with a guestbook and the audio previewer installed locally. Looking at the javascript console, the callback url doesn't appear to have the new guestbookresponse id parameter:

http://localhost:8080/previewers/v1.5/AudioPreview.html?callback=aHR0cDovLzEyNy4wLjAuMTo4MDgwL2FwaS92MS9maWxlcy8xNi9tZXRhZGF0YS8yL3Rvb2xwYXJhbXMvNA==&locale=en&preview=true

decoded:

echo "aHR0cDovLzEyNy4wLjAuMTo4MDgwL2FwaS92MS9maWxlcy8xNi9tZXRhZGF0YS8yL3Rvb2xwYXJhbXMvNA==" | base64 -d 
http://127.0.0.1:8080/api/v1/files/16/metadata/2/toolparams/4

This in turn produces an /access url without the gbrid, which bombs with a 400.

Please advise.

[stevenwinship]

Did you install your own external tool? If so did you include "gbrecs=false" or "gbrecs=true" in the allowedapicalls?
I don't see it in your urls above.

This is what it looks for to add the gb response id. I'll look into changing it so it gets added either way

[{"name":"retrieveFileContents","httpMethod":"GET","urlTemplate":"/api/v1/access/datafile/{fileId}?gbrecs=true","timeOut":3600},{"name":"downloadFile","httpMethod":"GET","urlTemplate":"/api/v1/access/datafile/{fileId}?gbrecs=false","timeOut":3600},{"name":"getDatasetVersionMetadata","httpMethod":"GET","urlTemplate":"/api/v1/datasets/{datasetId}/versions/{datasetVersion}","timeOut":3600}]

[landreev]

I installed the tool using the standard command as published. Which in my case was:

curl -X POST -H 'Content-type: application/json' http://localhost:8080/api/admin/externalTools -d \
'{                                            
  "displayName":"Play Audio",    
  "description":"Listen to an audio file.",
  "toolName":"audioPreviewer",
  "scope":"file",                    
  "types":["preview"],    
  "toolUrl":"http://localhost:8080/previewers/v1.5/AudioPreview.html",
  "toolParameters": {                
      "queryParameters":[ 
        {"fileid":"{fileId}"},                                      
        {"siteUrl":"{siteUrl}"},
        {"datasetid":"{datasetId}"},
        {"datasetversion":"{datasetVersion}"},                       
        {"locale":"{localeCode}"}
      ]                   
    },                                                               
  "contentType":"audio/mpeg",             
  "allowedApiCalls": [    
    {                                                                         
      "name": "retrieveFileContents",     
      "httpMethod": "GET",
      "urlTemplate": "/api/v1/access/datafile/{fileId}?gbrecs=true",          
      "timeOut": 3600 
    },                    
    {                                                               
      "name": "downloadFile",   
      "httpMethod": "GET",          
      "urlTemplate": "/api/v1/access/datafile/{fileId}?gbrecs=false",
      "timeOut": 3600            
    },                    
    {                                                                
      "name": "getDatasetVersionMetadata",
      "httpMethod": "GET",
      "urlTemplate": "/api/v1/datasets/{datasetId}/versions/{datasetVersion}",
      "timeOut": 3600                     
    }                     
  ]                                                                           
}'

seems to be the same - i.e., gbrecs=true for the retrieveFileContents (the main access url that the player uses), and gbrecs=false for the download one, just like in your command below. (but please double-check)
If the installation commands need to be changed/tools need to be reinstalled, that will need to be documented in the release note.

Did you install your own external tool? If so did you include "gbrecs=false" or "gbrecs=true" in the allowedapicalls? This is what it looks for to add the gb id. I'll look into changed it so it gets added either way

[{"name":"retrieveFileContents","httpMethod":"GET","urlTemplate":"/api/v1/access/datafile/{fileId}?gbrecs=true","timeOut":3600},{"name":"downloadFile","httpMethod":"GET","urlTemplate":"/api/v1/access/datafile/{fileId}?gbrecs=false","timeOut":3600},{"name":"getDatasetVersionMetadata","httpMethod":"GET","urlTemplate":"/api/v1/datasets/{datasetId}/versions/{datasetVersion}","timeOut":3600}]

[landreev]

Do I need to re-test, with your latest commits from 1 hour ago? Or are these the same initial commits from force-pushing the branch?

I'm getting errors/warning from git pull origin 12760-previewers-not-working-with-guestbook-responses as well:

hint: You have divergent branches and need to specify how to reconcile them.
hint: You can do so by running one of the following commands sometime before
hint: your next pull:
...

[landreev]

... otherwise I can report another issue that I may be seeing.

[stevenwinship]

My latest commit was just a merge from develop

[landreev]

I figured; but they are showing as new commits this am, above. I'm assuming because of a force-push?

Since we do want/need to have the gbrids= in the main access url, it should probably have gbrecs=false, not true - (false means "do not write a new gbr", right?)

[qqmyers]

FWIW: ignoring when a gb is assigned, gbrecs determines whether an entry is made that signifies a download, so the main preview needs/needed true to avoid the API writing a download count. At some point, we decided that a preview is close enough to a download that we want to ask the guestbook questions, but I don't think we ever decided that, in the no guestbook case, previews should count as downloads.

[landreev]

In other words, I was wrong above - in that gbrecs=true actually means "do NOT write a new gbr"?

On a side note, I'm not seeing either gbrecs or gbrids mentioned anywhere in the guides. (am I missing something?)

FWIW: ignoring when a gb is assigned, gbrecs determines whether an entry is made that signifies a download, so the main preview needs/needed true to avoid the API writing a download count. At some point, we decided that a preview is close enough to a download that we want to ask the guestbook questions, but I don't think we ever decided that, in the no guestbook case, previews should count as downloads.

[landreev]

In other words, I was wrong above - in that gbrecs=true actually means "do NOT write a new gbr"?

Of course; I misread the code in Access.java earlier.

[stevenwinship]

I've been trying to reproduce your (@landreev) error using both .wav and .mpeg files. I am getting the dialog to enter the gb info and then I get the download to happen correctly with the id

{"name":"downloadFile","httpMethod":"GET","urlTemplate":"/api/v1/access/datafile/{fileId}?gbrecs=true&gbrids=4","timeOut":3600},

Note: I can preview the wav file but I don't see the "eye" with the mpeg

[stevenwinship]

With this latest commit the url given to the external tool for download purposes will expire in 60 minutes. The signed url still retains the timeout from the tools configuration but it will not be honored beyond the 60 minutes.
ie. the signed part has a timeout equal to the configuration but the Access API endpoint will block if over 60 minutes. If the api gets called by a download (not external tool) the timeout remains at 1 minute since this is a two pass POST/GET from the ui and doesn't take much time to complete.

The external tool download will still create "Explore" and "View Image" guestbook responses (when clicking preview(EYE) and "Open in new window") but will not generate the "Download" response.

EDIT: I don't think the "View Image" matters. Seems it's just the name of the previewer. So it doesn't seem to have a negative effect on the download count.
/** Event Types - there are four pre-defined values in use.
* The type can also be the name of a previewer/explore tool
*/

[landreev]

Sorry for the delay. Re-testing and looking at the code now; especially interested r/n in what I can get away with as a guest user.
Will try to wrap it up soon.

[landreev]

The previewers are now working on files w/ explicit guestbooks, for an hour, as planned.
My only request is let's make both GUESTBOOK_RESPONSE_SIGNEDURL_TIMEOUT_MINUTES and GUESTBOOK_RESPONSE_EXTERNAL_TOOL_TIMEOUT_MINUTES configurable, via dataverse.api.* jvm options, with the current values as the defaults.
If an instance has a reason to want either of these to be a week, or 5 minutes, they should have a way to have that.

I also have an observation that I want to document, will do separately.

[landreev]

What is the bot complaining about here, the "Cognitive Complexity" business - does it have any merit?

[stevenwinship]

I removed 1 of the unused method params. I don't know who decided that 15 was a limit. I'm not removing what's needed or creating a class to pass multiple values.

[landreev]

I'd like to make an effort and merge this PR asap.
But what about this one remaining request? - I would like to have these configurable.

The previewers are now working on files w/ explicit guestbooks, for an hour, as planned. My only request is let's make both GUESTBOOK_RESPONSE_SIGNEDURL_TIMEOUT_MINUTES and GUESTBOOK_RESPONSE_EXTERNAL_TOOL_TIMEOUT_MINUTES configurable, via dataverse.api.* jvm options, with the current values as the defaults. If an instance has a reason to want either of these to be a week, or 5 minutes, they should have a way to have that.

I also have an observation that I want to document, will do separately.

[landreev]

(Sorry if you are still working on it - I didn't see your name assigned and assumed it was ready for retesting)

[landreev]

a lost double quote?

[stevenwinship]

yes.

[stevenwinship]

fixed

[sonarqubecloud]

Quality Gate failed

Failed conditions
14.4% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

[github-actions]

📦 Pushed preview images as

ghcr.io/gdcc/dataverse:12760-previewers-not-working-with-guestbook-responses

ghcr.io/gdcc/configbaker:12760-previewers-not-working-with-guestbook-responses

🚢 See on GHCR. Use by referencing with full name as printed above, mind the registry name.