Skip to content

Commit

Permalink
Ensure datatypes match on dragging elements into FormData
Browse files Browse the repository at this point in the history
This is pretty common source of cheetah templating issues
where tool authors assume (orrectly, IMO) only datatypes allowed in the format
attribute of an input dataset are allowed.
Here's a hypothetical example:

```
  # set index = $input.cram_index
  # set index = $input.cram_index
ln -s '$index' input.index
```
This is going to fail if a user drag e.g. a txt dataset into the input.

For anything but the valid datatypes this is going to fail.

Another way things can fail is if an input datatypes should contain
extra files, but doesn't, for instance in
https://sentry.galaxyproject.org/share/issue/73d7bd51dd2d418e96d563f0ac2383f0/:

```
Exception: __action_for_transfer called on non-existent file - [None]
  File "galaxy/jobs/runners/pulsar.py", line 446, in queue_job
    external_job_id = pulsar_submit_job(client, client_job_description, remote_job_config)
  File "pulsar/client/staging/up.py", line 39, in submit_job
    file_stager = FileStager(client, client_job_description, job_config)
  File "pulsar/client/staging/up.py", line 148, in __init__
    self.__upload_input_files()
  File "pulsar/client/staging/up.py", line 264, in __upload_input_files
    self.__upload_input_metadata_file(client_input.action_source)
  File "pulsar/client/staging/up.py", line 287, in __upload_input_metadata_file
    self.transfer_tracker.handle_transfer_source(input_action_source, path_type.INPUT, name=remote_name)
  File "pulsar/client/staging/up.py", line 495, in handle_transfer_source
    action = self.__action_for_transfer(source, type, contents)
  File "pulsar/client/staging/up.py", line 551, in __action_for_transfer
    raise Exception(message)
```

Fixes #8202

We can now easily change datatypes in batch if necessary,
this shouldn't be an argument to not properly implement
datatype filtering.
  • Loading branch information
mvdbeek committed Jun 16, 2024
1 parent 4fd73fc commit 486f413
Show file tree
Hide file tree
Showing 4 changed files with 65 additions and 18 deletions.
4 changes: 4 additions & 0 deletions client/src/components/Form/Elements/FormData/FormData.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import { mount } from "@vue/test-utils";
import { PiniaVuePlugin } from "pinia";
import { dispatchEvent, getLocalVue } from "tests/jest/helpers";

import { testDatatypesMapper } from "@/components/Datatypes/test_fixtures";
import { useDatatypesMapperStore } from "@/stores/datatypesMapperStore";
import { useEventStore } from "@/stores/eventStore";

import MountTarget from "./FormData.vue";
Expand All @@ -15,6 +17,8 @@ let eventStore;
function createTarget(propsData) {
const pinia = createTestingPinia({ stubActions: false });
eventStore = useEventStore();
const datatypesStore = useDatatypesMapperStore();
datatypesStore.datatypesMapper = testDatatypesMapper;
return mount(MountTarget, {
localVue,
propsData,
Expand Down
56 changes: 45 additions & 11 deletions client/src/components/Form/Elements/FormData/FormData.vue
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { BAlert, BButton, BButtonGroup, BCollapse, BFormCheckbox, BTooltip } fro
import { computed, onMounted, type Ref, ref, watch } from "vue";
import { getGalaxyInstance } from "@/app";
import { useDatatypesMapper } from "@/composables/datatypesMapper";
import { useUid } from "@/composables/utils/uid";
import { type EventData, useEventStore } from "@/stores/eventStore";
import { orList } from "@/utils/strings";
Expand Down Expand Up @@ -51,8 +52,9 @@ const props = withDefaults(
);
const eventStore = useEventStore();
const { datatypesMapper } = useDatatypesMapper();
const $emit = defineEmits(["input"]);
const $emit = defineEmits(["input", "alert"]);
// Determines wether values should be processed as linked or unlinked
const currentLinked = ref(true);
Expand Down Expand Up @@ -302,6 +304,10 @@ function getSourceType(val: DataOption) {
function handleIncoming(incoming: Record<string, unknown>, partial = true) {
if (incoming) {
const values = Array.isArray(incoming) ? incoming : [incoming];
const extensions = values.map((v) => v.extension || v.elements_datatypes).filter((v) => (v ? true : false));
if (!canAcceptDatatype(extensions)) {
return false;
}
if (values.length > 0) {
const incomingValues: Array<DataOption> = [];
values.forEach((v) => {
Expand Down Expand Up @@ -349,6 +355,7 @@ function handleIncoming(incoming: Record<string, unknown>, partial = true) {
}
}
}
return true;
}
/**
Expand All @@ -372,10 +379,36 @@ function onBrowse() {
}
}
function canAcceptDatatype(itemDatatypes: string | Array<string>) {
if (!(props.extensions?.length > 0)) {
return true;
}
let datatypes: Array<string>;
if (!Array.isArray(itemDatatypes)) {
datatypes = [itemDatatypes];
} else {
datatypes = itemDatatypes;
}
const incompatibleItem = datatypes.find(
(extension) => !datatypesMapper.value?.isSubTypeOfAny(extension, props.extensions)
);
if (incompatibleItem) {
return false;
}
return true;
}
// Drag/Drop event handlers
function onDragEnter(evt: MouseEvent) {
const eventData = eventStore.getDragData();
if (eventData) {
const extensions = (eventData.extension as string) || (eventData.elements_datatypes as Array<string>);
if (!canAcceptDatatype(extensions)) {
currentHighlighting.value = "warning";
$emit("alert", `${extensions} is not an acceptable format for this parameter.`);
} else {
currentHighlighting.value = "success";
}
dragTarget.value = evt.target;
dragData.value = eventData;
}
Expand All @@ -384,23 +417,24 @@ function onDragEnter(evt: MouseEvent) {
function onDragLeave(evt: MouseEvent) {
if (dragTarget.value === evt.target) {
currentHighlighting.value = null;
}
}
function onDragOver() {
if (dragData.value !== null) {
currentHighlighting.value = "warning";
$emit("alert", undefined);
}
}
function onDrop() {
if (dragData.value) {
let accept = false;
if (eventStore.multipleDragData) {
handleIncoming(Object.values(dragData.value) as any, false);
accept = handleIncoming(Object.values(dragData.value) as any, false);
} else {
accept = handleIncoming(dragData.value);
}
if (accept) {
currentHighlighting.value = "success";
} else {
handleIncoming(dragData.value);
currentHighlighting.value = "warning";
}
currentHighlighting.value = "success";
$emit("alert", undefined);
dragData.value = null;
clearHighlighting();
}
Expand Down Expand Up @@ -468,7 +502,7 @@ const noOptionsWarningMessage = computed(() => {
:class="currentHighlighting && `ui-dragover-${currentHighlighting}`"
@dragenter.prevent="onDragEnter"
@dragleave.prevent="onDragLeave"
@dragover.prevent="onDragOver"
@dragover.prevent
@drop.prevent="onDrop">
<div class="d-flex flex-column">
<BButtonGroup v-if="variant && variant.length > 1" buttons class="align-self-start">
Expand Down
2 changes: 1 addition & 1 deletion client/src/components/Form/FormElement.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ describe("FormElement", () => {
const error = wrapper.find(".ui-form-error-text");
expect(error.text()).toBe("error_text");

await wrapper.setProps({ error: "" });
await wrapper.setProps({ error: undefined });
const no_error = wrapper.findAll(".ui-form-error");
expect(no_error.length).toBe(0);

Expand Down
21 changes: 15 additions & 6 deletions client/src/components/Form/FormElement.vue
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ function onConnect() {
const isHidden = computed(() => attrs.value["hidden"]);
const elementId = computed(() => `form-element-${props.id}`);
const hasAlert = computed(() => Boolean(props.error || props.warning));
const hasAlert = computed(() => alerts.value.length > 0);
const showPreview = computed(() => (collapsed.value && attrs.value["collapsible_preview"]) || props.disabled);
const showField = computed(() => !collapsed.value && !props.disabled);
Expand Down Expand Up @@ -174,6 +174,16 @@ const isEmpty = computed(() => {
const isRequired = computed(() => attrs.value["optional"] === false);
const isRequiredType = computed(() => props.type !== "boolean");
const isOptional = computed(() => !isRequired.value && attrs.value["optional"] !== undefined);
const formAlert = ref<string>();
const alerts = computed(() => {
return [formAlert.value, props.error, props.warning]
.filter((v) => v !== undefined && v !== null)
.map((v) => linkify(sanitize(v!, { USE_PROFILES: { html: true } })));
});
function onAlert(value: string | undefined) {
formAlert.value = value;
}
</script>

<template>
Expand All @@ -182,11 +192,9 @@ const isOptional = computed(() => !isRequired.value && attrs.value["optional"] !
:id="elementId"
class="ui-form-element section-row"
:class="{ alert: hasAlert, 'alert-info': hasAlert }">
<div v-if="hasAlert" class="ui-form-error">
<div v-for="(alert, index) in alerts" :key="index" class="ui-form-error">
<FontAwesomeIcon class="mr-1" icon="fa-exclamation" />
<span
class="ui-form-error-text"
v-html="linkify(sanitize(props.error || props.warning, { USE_PROFILES: { html: true } }))" />
<span class="ui-form-error-text" v-html="alert" />
</div>

<div class="ui-form-title">
Expand Down Expand Up @@ -288,7 +296,8 @@ const isOptional = computed(() => !isRequired.value && attrs.value["optional"] !
:optional="attrs.optional"
:options="attrs.options"
:tag="attrs.tag"
:type="props.type" />
:type="props.type"
@alert="onAlert" />
<FormDrilldown
v-else-if="props.type === 'drill_down'"
:id="id"
Expand Down

0 comments on commit 486f413

Please sign in to comment.