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

Problem with long file stores ID in JVM options #8312

Closed
fredericbeck opened this issue Dec 16, 2021 · 8 comments · Fixed by #10031
Closed

Problem with long file stores ID in JVM options #8312

fredericbeck opened this issue Dec 16, 2021 · 8 comments · Fixed by #10031
Assignees
Labels
Feature: File Upload & Handling Hackathon: Low Hanging Fruit hacktoberfest It's Hacktoberfest! https://groups.google.com/g/dataverse-community/c/n_Nn_T2yA-w/m/BcoXO4tEAQAJ Help Wanted: Code Mentor: pdurbin Type: Bug a defect
Milestone

Comments

@fredericbeck
Copy link

What steps does it take to reproduce the issue?

  • When does this issue occur?

When adding a file store with a long name in the JVM options, file uploads fail whatever the upload method used, generating a javax.ejb.EJBException: String index out of range: -3 error in the server logs.

Everything works for short ID, e.g. sed-nge-storage or dv-global, but not for longer ones such as dataverse-global-storage

  • Which page(s) does it occurs on?

Error occur on the file upload page, or when using DV Uploader or direct curl API access.

  • What happens?

File upload fails, detailed error log can be found here.

  • To whom does it occur (all users, curators, superusers)?

All users are impacted.

  • Workaround

Using shorter file store IDs works fine

Which version of Dataverse are you using?

Using Dataverse v5.8

@qqmyers
Copy link
Member

qqmyers commented Dec 16, 2021

@fredericbeck - thanks! Having looked after chat yesterday, I'm fairly sure that the problem with the line @pdurbin referenced -

if(!storageIdentifier.substring((this.driverId + "://").length()).contains(":")) {
is that it should just be if(!storageIdentifier.contains(":")) { .

I think this is 'just' a bug - a cut/paste error. It will cause problems whenever the length of the store name is within a couple characters of being longer than the bucket name.

@pdurbin
Copy link
Member

pdurbin commented Oct 1, 2022

@fredericbeck are you interested in creating a pull request for this? We don't want to deprive you of long and meaningful IDs for stores. 😄

@pdurbin pdurbin added Feature: File Upload & Handling Type: Bug a defect Hackathon: Low Hanging Fruit Help Wanted: Code Mentor: pdurbin hacktoberfest It's Hacktoberfest! https://groups.google.com/g/dataverse-community/c/n_Nn_T2yA-w/m/BcoXO4tEAQAJ labels Oct 1, 2022
@pdurbin
Copy link
Member

pdurbin commented Oct 1, 2022

@qqmyers to be clear, we're saying this only affects the S3 driver, right? The file system driver should be ok?

@AR-2910
Copy link
Contributor

AR-2910 commented Oct 21, 2023

Hey @pdurbin! I would like to address this issue and implement the solution provided by @qqmyers. Could you please assign it to me?

Regards

@pdurbin
Copy link
Member

pdurbin commented Oct 22, 2023

@AR-2910 it looks like @qqmyers assigned you. Thanks!

@AR-2910
Copy link
Contributor

AR-2910 commented Oct 22, 2023

hey @pdurbin @qqmyers ,
I've reviewed the code related to this issue. I noticed that the specific line to change is different from what the issue originally asked for. The line in question is as follows:
if (!storageIdentifier.substring((this.driverId + DataAccess.SEPARATOR).length()).contains(":")) {

This differs from the proposed change in the issue description. The issue initially requested changing it to:
if (!storageIdentifier.contains(":")) {

The current code appears to have evolved, and the context of the check has changed. To address the issue and align with the current codebase, I plan to implement the proposed change line that does not involve substring. This change aims to simplify the code and make it more robust

Before proceeding, I wanted to confirm the correct line to modify. Could you please provide clarification on which change should be made?

Regards

@qqmyers
Copy link
Member

qqmyers commented Oct 22, 2023

Looks like that line moved to

if(!storageIdentifier.substring((this.driverId + DataAccess.SEPARATOR).length()).contains(":")) {

AR-2910 added a commit to AR-2910/dataverse that referenced this issue Oct 22, 2023
Issue: Problem with long file stores ID in JVM options IQSS#8312

Change made in line 194.
@AR-2910
Copy link
Contributor

AR-2910 commented Oct 22, 2023

Hey @qqmyers @pdurbin,

Thank you for assigning issue #8312 to me. I appreciate the opportunity to contribute to this project.

I have created a pull request #10031 for the code change.

Please review the pull request at your convenience. If the changes look good and meet the requirements, I would appreciate you merging it into the main branch.

Regards

kcondon added a commit that referenced this issue Oct 26, 2023
Problem with long file stores ID in JVM options #8312
@pdurbin pdurbin added this to the 6.1 milestone Oct 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature: File Upload & Handling Hackathon: Low Hanging Fruit hacktoberfest It's Hacktoberfest! https://groups.google.com/g/dataverse-community/c/n_Nn_T2yA-w/m/BcoXO4tEAQAJ Help Wanted: Code Mentor: pdurbin Type: Bug a defect
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants