-
Notifications
You must be signed in to change notification settings - Fork 493
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
5028 Add dataset level external tools #6059
Changes from 2 commits
f699fd6
48d1181
0390e3c
6fe2996
e9dad22
d8f7a96
c0df37d
162207e
319141a
3d522aa
cc99058
952c30b
336a424
8075283
94f8127
e92c8f1
969d5ed
f8e1e0e
801bf56
e2e34d3
1578595
50d6cda
c89819a
4897de4
7828dea
760d670
6dd0cfc
519662a
09766f1
06105e8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,8 +6,10 @@ | |
import edu.harvard.iq.dataverse.authorization.AuthenticationServiceBean; | ||
import edu.harvard.iq.dataverse.authorization.Permission; | ||
import edu.harvard.iq.dataverse.authorization.providers.builtin.BuiltinUserServiceBean; | ||
import edu.harvard.iq.dataverse.authorization.users.ApiToken; | ||
import edu.harvard.iq.dataverse.authorization.users.AuthenticatedUser; | ||
import edu.harvard.iq.dataverse.authorization.users.PrivateUrlUser; | ||
import edu.harvard.iq.dataverse.authorization.users.User; | ||
import edu.harvard.iq.dataverse.branding.BrandingUtil; | ||
import edu.harvard.iq.dataverse.dataaccess.StorageIO; | ||
import edu.harvard.iq.dataverse.dataaccess.ImageThumbConverter; | ||
|
@@ -99,6 +101,7 @@ | |
import edu.harvard.iq.dataverse.externaltools.ExternalTool; | ||
import edu.harvard.iq.dataverse.externaltools.ExternalToolServiceBean; | ||
import edu.harvard.iq.dataverse.export.SchemaDotOrgExporter; | ||
import edu.harvard.iq.dataverse.externaltools.ExternalToolHandler; | ||
import edu.harvard.iq.dataverse.makedatacount.MakeDataCountLoggingServiceBean; | ||
import edu.harvard.iq.dataverse.makedatacount.MakeDataCountLoggingServiceBean.MakeDataCountEntry; | ||
import java.util.Collections; | ||
|
@@ -135,6 +138,7 @@ | |
import org.apache.solr.client.solrj.response.QueryResponse; | ||
import org.apache.solr.common.SolrDocument; | ||
import org.apache.solr.common.SolrDocumentList; | ||
import org.primefaces.PrimeFaces; | ||
import org.primefaces.model.DefaultTreeNode; | ||
import org.primefaces.model.TreeNode; | ||
|
||
|
@@ -316,6 +320,7 @@ public void setShowIngestSuccess(boolean showIngestSuccess) { | |
List<ExternalTool> exploreTools = new ArrayList<>(); | ||
Map<Long, List<ExternalTool>> configureToolsByFileId = new HashMap<>(); | ||
Map<Long, List<ExternalTool>> exploreToolsByFileId = new HashMap<>(); | ||
private List<ExternalTool> datasetExploreTools; | ||
|
||
public Boolean isHasRsyncScript() { | ||
return hasRsyncScript; | ||
|
@@ -2016,6 +2021,7 @@ private String init(boolean initFull) { | |
|
||
configureTools = externalToolService.findByType(ExternalTool.Type.CONFIGURE); | ||
exploreTools = externalToolService.findByType(ExternalTool.Type.EXPLORE); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. findByType isn't filtering by scope yet - should these use findByTypeAndScope with file scope? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, they probably should. |
||
datasetExploreTools = externalToolService.findByScopeAndType(ExternalTool.Scope.DATASET, ExternalTool.Type.EXPLORE); | ||
rowsPerPage = 10; | ||
|
||
|
||
|
@@ -5052,6 +5058,10 @@ public List<ExternalTool> getCachedToolsForDataFile(Long fileId, ExternalTool.Ty | |
return cachedTools; | ||
} | ||
|
||
public List<ExternalTool> getDatasetExploreTools() { | ||
return datasetExploreTools; | ||
} | ||
|
||
Boolean thisLatestReleasedVersion = null; | ||
|
||
public boolean isThisLatestReleasedVersion() { | ||
|
@@ -5200,4 +5210,17 @@ public int compare(FileMetadata o1, FileMetadata o2) { | |
return type1.compareTo(type2); | ||
} | ||
}; | ||
|
||
public void explore(ExternalTool externalTool) { | ||
ApiToken apiToken = null; | ||
User user = session.getUser(); | ||
if (user instanceof AuthenticatedUser) { | ||
apiToken = authService.findApiTokenByUser((AuthenticatedUser) user); | ||
} | ||
ExternalToolHandler externalToolHandler = new ExternalToolHandler(externalTool, dataset, apiToken); | ||
String toolUrl = externalToolHandler.getToolUrlWithQueryParams(); | ||
logger.fine("Exploring with " + toolUrl); | ||
PrimeFaces.current().executeScript("window.open('"+toolUrl + "', target='_blank');"); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,12 +4,14 @@ | |
import edu.harvard.iq.dataverse.DataFileServiceBean; | ||
import edu.harvard.iq.dataverse.externaltools.ExternalTool.ReservedWord; | ||
import edu.harvard.iq.dataverse.externaltools.ExternalTool.Type; | ||
import edu.harvard.iq.dataverse.externaltools.ExternalTool.Scope; | ||
|
||
import static edu.harvard.iq.dataverse.externaltools.ExternalTool.DESCRIPTION; | ||
import static edu.harvard.iq.dataverse.externaltools.ExternalTool.DISPLAY_NAME; | ||
import static edu.harvard.iq.dataverse.externaltools.ExternalTool.TOOL_PARAMETERS; | ||
import static edu.harvard.iq.dataverse.externaltools.ExternalTool.TOOL_URL; | ||
import static edu.harvard.iq.dataverse.externaltools.ExternalTool.TYPE; | ||
import static edu.harvard.iq.dataverse.externaltools.ExternalTool.SCOPE; | ||
import static edu.harvard.iq.dataverse.externaltools.ExternalTool.CONTENT_TYPE; | ||
import java.io.StringReader; | ||
import java.util.ArrayList; | ||
|
@@ -74,7 +76,21 @@ public List<ExternalTool> findByType(Type type, String contentType) { | |
return externalTools; | ||
} | ||
|
||
|
||
/** | ||
* @param scope - dataset or file | ||
* @return A list of tools or an empty list. | ||
*/ | ||
public List<ExternalTool> findByScopeAndType(Scope scope, Type type) { | ||
List<ExternalTool> externalTools = new ArrayList<>(); | ||
TypedQuery<ExternalTool> typedQuery = em.createQuery("SELECT OBJECT(o) FROM ExternalTool AS o WHERE o.scope = :scope AND o.type = :type", ExternalTool.class); | ||
typedQuery.setParameter("scope", scope); | ||
typedQuery.setParameter("type", type); | ||
List<ExternalTool> toolsFromQuery = typedQuery.getResultList(); | ||
if (toolsFromQuery != null) { | ||
externalTools = toolsFromQuery; | ||
} | ||
return externalTools; | ||
} | ||
|
||
public ExternalTool findById(long id) { | ||
TypedQuery<ExternalTool> typedQuery = em.createQuery("SELECT OBJECT(o) FROM ExternalTool AS o WHERE o.id = :id", ExternalTool.class); | ||
|
@@ -131,6 +147,7 @@ public static ExternalTool parseAddExternalToolManifest(String manifest) { | |
String displayName = getRequiredTopLevelField(jsonObject, DISPLAY_NAME); | ||
String description = getRequiredTopLevelField(jsonObject, DESCRIPTION); | ||
String typeUserInput = getRequiredTopLevelField(jsonObject, TYPE); | ||
String scopeUserInput = getRequiredTopLevelField(jsonObject, SCOPE); | ||
String contentType = getOptionalTopLevelField(jsonObject, CONTENT_TYPE); | ||
//Legacy support - assume tool manifests without any mimetype are for tabular data | ||
if(contentType==null) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is going to set a content type on dataset level tools There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this is a bug. We don't want dataset tools to have a content type. Good catch. Thanks. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @qqmyers eventually we may use this - for example, show the dataset level explore only if the dataset has at least one file of content type X - but for now we're going with simple. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I understand @scolapasta's suggestion - we may have a use for the content type, with some dataset-level external tools; but it sounds like it still needs to be nullable in the table; and it still should be null by default. |
||
|
@@ -139,26 +156,29 @@ public static ExternalTool parseAddExternalToolManifest(String manifest) { | |
|
||
// Allow IllegalArgumentException to bubble up from ExternalTool.Type.fromString | ||
ExternalTool.Type type = ExternalTool.Type.fromString(typeUserInput); | ||
ExternalTool.Scope scope = ExternalTool.Scope.fromString(scopeUserInput); | ||
String toolUrl = getRequiredTopLevelField(jsonObject, TOOL_URL); | ||
JsonObject toolParametersObj = jsonObject.getJsonObject(TOOL_PARAMETERS); | ||
JsonArray queryParams = toolParametersObj.getJsonArray("queryParameters"); | ||
boolean allRequiredReservedWordsFound = false; | ||
for (JsonObject queryParam : queryParams.getValuesAs(JsonObject.class)) { | ||
Set<String> keyValuePair = queryParam.keySet(); | ||
for (String key : keyValuePair) { | ||
String value = queryParam.getString(key); | ||
ReservedWord reservedWord = ReservedWord.fromString(value); | ||
if (reservedWord.equals(ReservedWord.FILE_ID)) { | ||
allRequiredReservedWordsFound = true; | ||
if (scope.equals(Scope.FILE)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. else Scope.DATASET - just not written yet? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, I'm not sure. Should dataset tools have any reserved words that are required? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. without an else statement, allRequiredReservedWordsFound is false and an exception happens so this method doesn't work for scope dataset as is. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah sorry - I see the exception is in the if statement - probably should not define the allRequiredWordsFound outside the if clause then (to avoid confusing people like me :-) ) |
||
for (JsonObject queryParam : queryParams.getValuesAs(JsonObject.class)) { | ||
Set<String> keyValuePair = queryParam.keySet(); | ||
for (String key : keyValuePair) { | ||
String value = queryParam.getString(key); | ||
ReservedWord reservedWord = ReservedWord.fromString(value); | ||
if (reservedWord.equals(ReservedWord.FILE_ID)) { | ||
allRequiredReservedWordsFound = true; | ||
} | ||
} | ||
} | ||
} | ||
if (!allRequiredReservedWordsFound) { | ||
// Some day there might be more reserved words than just {fileId}. | ||
throw new IllegalArgumentException("Required reserved word not found: " + ReservedWord.FILE_ID.toString()); | ||
if (!allRequiredReservedWordsFound) { | ||
// Some day there might be more reserved words than just {fileId}. | ||
throw new IllegalArgumentException("Required reserved word not found: " + ReservedWord.FILE_ID.toString()); | ||
} | ||
} | ||
String toolParameters = toolParametersObj.toString(); | ||
return new ExternalTool(displayName, description, type, toolUrl, toolParameters, contentType); | ||
return new ExternalTool(displayName, description, type, scope, toolUrl, toolParameters, contentType); | ||
} | ||
|
||
private static String getRequiredTopLevelField(JsonObject jsonObject, String key) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
ALTER TABLE externaltool ADD COLUMN IF NOT EXISTS scope VARCHAR(255); | ||
UPDATE externaltool SET scope = 'FILE'; | ||
ALTER TABLE externaltool ALTER COLUMN scope SET NOT NULL; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should content type now be nullable (in the db and class definition) since it isn't used for scope dataset? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As mentioned above, due to a bug in the pull request, dataset tools get the default content type. Yes, I think this field should be made nullable (and the bug fixed). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So what is the status of this - is this being addressed? I.e., is the bug in ExternalToolServiceBean getting fixed in this PR, and is ContentType being made nullable? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this and not file PID too? Since Dataverse is always generating this, I'm not sure it helps anyone to have both id and pid (a tool can get the pid), but it seems like file and dataset should be the same.
Also - right now we check for fileId existing (for file scope) as a required condition. For dataset, and file if it is made parallel, the condition will have to change to be that id or pid is required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. The focus right now is on dataset level tools but sure, if a file PID exists it would hurt to expose it. Tool makers should just understand that they might not always get a file PID.
I'm not sure if I grok your other comment but thanks for making it. 😄 Someone else will be picking this up while I'm on vacation, I suspect.