BE: add based_on_upload to GET /api/collections/:collection/items response
#38322
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* update upload function to take arguments as an object * upload to model from context menu * add e2e tests for appends * update unit tests * clean up * fix types * ellipsify status title * don't restrict appends to models on the frontend * move upload button out of context menu * automatically show upload error modal * update tests * better data fetching and selecting
|
qnkhuat
reviewed
Feb 5, 2024
src/metabase/upload.clj
Outdated
| (let [table-ids (->> models | ||
| ;; as an optimization, we might already know that the table is not an upload | ||
| ;; if is_upload=false. We can skip making more queries if so | ||
| (remove #(false? (:is_upload %))) |
There was a problem hiding this comment.
in which case we benefit from this optimization?
There was a problem hiding this comment.
The code path from the collection items endpoint. I'll add this to the comment
qnkhuat
reviewed
Feb 5, 2024
| (:id t))) | ||
| tables))))) | ||
|
|
||
| (defn- no-joins? |
There was a problem hiding this comment.
Suggested change
| (defn- no-joins? | |
| (defn- query-no-joins? |
IMO, if the object that a helper function operates on is not the main object of the namespace, we should prefix it.
For example, it doesn't give much context if you call (upload/no-joins ...) since upload doesn't have a join property by itself.
There was a problem hiding this comment.
I can't see much benefit of applying that rule for private helper functions like this one which are only used in one function. I would agree with your example though, no one should use uploads/no-joins? without renaming it appropriately or moving it to an appropriate namespace.
There was a problem hiding this comment.
Agree on not needing a prefix for a private method (implicit in the arg name), but I do find this a strange name for a predicate. Having a has-joins? method that just gets negated at the call site feels a bit clearer to me.
qnkhuat
reviewed
Feb 5, 2024
src/metabase/upload.clj
Outdated
| [:table_id [:maybe ms/PositiveInt]] | ||
| ;; is_upload can be provided for an optional optimization | ||
| [:is_upload {:optional true} [:maybe :boolean]]]]] | ||
| (let [table-ids (->> models |
There was a problem hiding this comment.
nit: these lets binding are not aligned, and I noticed you no longer do that for some of your PRs. Have we stopped going that :( IMO, it aids readability by a good amount.
There was a problem hiding this comment.
I still align let bindings by default but sometimes break it if I think it leads to significantly more new lines. I can align this one though and can tend more towards aligning things in the future
qnkhuat
reviewed
Feb 5, 2024
| (contains? uploadable-table-ids (:table_id model)) | ||
| (no-joins? query)) | ||
| (lib/source-table-id query)))))] | ||
| (map #(m/assoc-some % :based_on_upload (based-on-upload %)) |
There was a problem hiding this comment.
Just thought it was cleaner. Why not?
There was a problem hiding this comment.
I'd expect based_on_upload to be a boolean key after calling model-hydrate-based-on-upload.
it'd be inconsistent if, for a list of models, some will have the key, and some are not
There was a problem hiding this comment.
Agreed it feels a little surprising. Using assoc here would mean based_on_upload would be not present for cards, but based_on_upload is present for models, which feels equally inconsistent to me.
There was a problem hiding this comment.
I slightly prefer using assoc here, as I think having the key uniformly present for all models is easier to reason about.
For a regular card we know the field is not relevant, but for models a response without the key could either be an implicit false, or just a response from an older version of the server. This is why it's good in general to have explicit nulls in server responses, since checking which keys were optional for a given past version will be painful when working on or providing support for old versions.
qnkhuat
reviewed
Feb 5, 2024
qnkhuat
reviewed
Feb 5, 2024
qnkhuat
reviewed
Feb 5, 2024
|
@calherries Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone? |
crisptrutski
reviewed
Feb 7, 2024
Comment on lines
+412
to
+416
| (map #(update % :dataset_query (comp mbql.normalize/normalize json/parse-string))) | ||
| upload/model-hydrate-based-on-upload | ||
| (map (fn [dataset_query row] | ||
| (assoc row :dataset_query dataset_query)) | ||
| dataset_queries) |
There was a problem hiding this comment.
This "normalize then undo" pattern could be made clearer IMHO. I see this has already shipped, so I'll send another PR for it.
crisptrutski
reviewed
Feb 7, 2024
| (defn uploadable-table-ids | ||
| "Returns the subset of table ids where the user can upload to the table." | ||
| [table-ids] | ||
| (if (empty? table-ids) |
There was a problem hiding this comment.
Does toucan / honeysql not have a fast "no query" return path for [:in []] queries? It just seems a bit sad to hand roll this optimization 😄
crisptrutski
reviewed
Feb 7, 2024
| [table-ids] | ||
| (if (empty? table-ids) | ||
| #{} | ||
| (let [tables (t2/hydrate (t2/select :model/Table :id [:in table-ids]) :db)] |
There was a problem hiding this comment.
nit: I think this would have been clearer with a threading macro:
(->> (t2/select ..)
t2/hydrate
(keep ...)
set)
crisptrutski
reviewed
Feb 7, 2024
| "Batch hydrates `:based_on_upload` for each item of `models`. Assumes each item of `model` represents a model." | ||
| [models :- [:sequential [:map | ||
| ;; query_type and dataset_query can be null in tests, so we make them nullable here. | ||
| ;; they should never be null in production |
There was a problem hiding this comment.
How sure are we really that it's never null in production?
I'd love to have a backlog item to backfill and make non-nullable, and update all the tests.
crisptrutski
reviewed
Feb 7, 2024
| ;; as an optimization when listing collection items (GET /api/collection/items), | ||
| ;; we might already know that the table is not an upload if is_upload=false. We | ||
| ;; can skip making more queries if so | ||
| (remove #(false? (:is_upload %))) |
There was a problem hiding this comment.
Is this relevant to the discussion about assoc versus assoc-with, or would there still be call paths that never populate the key for models?
crisptrutski
reviewed
Feb 7, 2024
| based-on-upload (fn [model] | ||
| (when-let [dataset_query (:dataset_query model)] ; dataset_query is sometimes null in tests | ||
| (let [query (lib/->pMBQL dataset_query)] | ||
| (when (and (= (name (:query_type model)) "query") |
There was a problem hiding this comment.
Should this not be "failing closed" for nil query_type? (= (name (:query_type model "query")) "query")
crisptrutski
reviewed
Feb 7, 2024
| (remove #(false? (:is_upload %))) | ||
| (keep :table_id) | ||
| set) | ||
| uploadable-table-ids (set (uploadable-table-ids table-ids)) |
There was a problem hiding this comment.
This method already returns a set.
crisptrutski
reviewed
Feb 7, 2024
| (keep :table_id) | ||
| set) | ||
| uploadable-table-ids (set (uploadable-table-ids table-ids)) | ||
| based-on-upload (fn [model] |
There was a problem hiding this comment.
I think it this case it would be more natural to have (for [m models] ...) in the body below this let, than to define this method to call in a map.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
epic
product doc
eng doc
This PR implements the BE part of milestone 1 of the epic.
It adds a
:based_on_uploadkey to the response of theGET /api/collections/:id/itemsandGET /api/collections/root/itemsendpoints.The
:based_on_upload keyis only added for items that correspond to models, not questions.The
:based_on_uploadis either:It reuses the same logic as #37052 to derive the
:based_on_uploadkey.