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

Null check/legacy code in DataFile.java #7849

Closed
landreev opened this issue May 4, 2021 · 0 comments · Fixed by #7868
Closed

Null check/legacy code in DataFile.java #7849

landreev opened this issue May 4, 2021 · 0 comments · Fixed by #7868
Assignees

Comments

@landreev
Copy link
Contributor

landreev commented May 4, 2021

TL;DR:
there's code in DataFile.java that checks if the file is harvested, based on whether the storageidentifier starts with http(s*)://, without a null check. To be safe, this needs to be addressed before #7741 makes it into a release.


The following code is in DataFile.java:

public boolean isHarvested() {
        
        // (storageIdentifier is not nullable - so no need to check for null
        // pointers below):
        if (this.getStorageIdentifier().startsWith("http://") || this.getStorageIdentifier().startsWith("https://")) {
            return true;
        }
        
        Dataset ownerDataset = this.getOwner();
        if (ownerDataset != null) {
            return ownerDataset.isHarvested(); 
        }
        return false; 
    }

The lines 3-7 are wrong on many levels. "storageidentifier is not nullable" is NOT true. (this is very old legacy code, left over from the time where the storageidentifier field lived in DataFile (and not in DvObject). The absence of the null check may become a problem in a legacy database fixed by the script in #7741. (Although there is a decent chance that our, Harvard prod. db is the only one affected).
Ideally, we should not rely on looking at the storageidentifier at all, in order to determine whether a file is harvested or not. (The code in the second half of the method above does the proper test - a harvested file is a file in a dataset that is harvested...).

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 a pull request may close this issue.

1 participant