Make format export optional

[adam-james-v]

Fixes: #40420
Related areas:

• direct download on Questions
• download results on Dashboard Cards
• formatting in alert/subscription attachments

Before this PR:

49 Introduced a nice change: exports now have formatting that matches the App. That is, visualization settings provided by admins and the users are respected in exports. We consider this a feature/improvement.

This did, however, change default outputs which is a surprise to some users who have built workflows and expectations around the minimal formatting that was provided in our exports previously. There was no way to opt-out of this new formatting except to recreate questions with visualization/column formatting explicitly set up to provide the desired output. This manual recreation isn't optimal in all cases.

After this PR:

This PR provides a URL parameter format_rows=false that allows API users to specify whether or not the 'App formatting' (what you see in the UI) is applied to the requested export.

All of the places the format_rows=false option can be used:

• /api/card/:card-id/query/:export-format
• /api/dataset/:export-format
• /api/dashboard/:dash-id/dashcard/:dashcard-id/card/:card-id/query/:export-format
• /api/public/card/:uuid/query/:export-format
• /api/embed/dashboard/:uuid/dashcard/:dashcard-id/card/:card-id/:export-format

Loom showing it working in various spots:
https://www.loom.com/share/35c135f846ff43958f8e43106ccdc2da?sid=c3c36612-c997-455e-90fe-e942e4ce34df

Here's a simple Clojure repl session/screenshot to illustrate the difference:

(ns tickets.unformatted
  (:require
   [clojure.data.csv :as csv]
   [clojure.test :refer :all]
   [toucan2.tools.with-temp :as t2.with-temp]))

;; Several endpoints have been altered to accept the boolean format_export parameter.
;; Here's the card endpoint for simplicity:

(defn explore
  []
  (let [q {:database (mt/id)
           :type     :native
           :native   {:query "SELECT 2000 AS number, '2024-03-26'::DATE AS date;"}}]
    (t2.with-temp/with-temp [Card {card-id :id} {:display :table :dataset_query q}]
      {:formatted
       (csv/read-csv (mt/user-http-request
                      :crowberto :post 200
                      (format "card/%d/query/csv" card-id)))
       :raw
       (csv/read-csv (mt/user-http-request
                      :crowberto :post 200
                      (format "card/%d/query/csv?format_rows=false" card-id)))})))

This change, paired with some frontend changes in: #40643 should give the option to turn off this formatting to users who need them in specific circumstances.

Since the format_export is defaulted to true in the middleware, we should see the improved formatting as the default everywhere, and turning it off becomes a specific choice users must make.

Format Rows as Middleware

I implemented this as a simple middleware. I think it makes sense alongside other middleware flags like :format-rows (which might be a good candidate for this :format_export to be merged into later), and process-viz-settings?. I considered passing a key into the qp.si/streaming-results-writer via viz-settings, but that felt like an abuse of the viz-settings map, and could be confusing as :format_export is not a key we intend to store in the app db, but is rather just a flag for the expected export behaviour during a single execution.

Todo:

• allow format_rows option to be used in subscriptions too
• fix default to 'true' (I think I mixed things up with Clojure's nil/false behaviour)
• apply the toggle to the xlsx exporter as well NOTE: xlsx formatting was already applied in prior versions, so leaving it alone for now
• add tests confirming correct behaviour for each of the 3 affected endpoints
• provide some examples in this PR
• explain the technical reason for how its implemented (below the fold in this PR)
• apply option to the related areas (tbd on exactly what this looks like) follow on frontend work in Make format export optional (FE part) #40643

[replay-io]

Status	Complete ↗︎
Commit	da7e634
Results	⚠️ 2 Flaky ✅ 2387 Passed

[dpsutton]

We already have a notion of :format-rows? and I worry that adding format-export? is adding extra knobs. Is it possible to unify these?

user=> (ns foo)
nil
foo=> (require '[clojure.data.csv :as csv]
               '[metabase.query-processor :as qp])
nil
foo=> (for [format? [true false]]
        (let [q {:database 1
                 :type     :native
                 :native   {:query "SELECT 2000 AS number, '2024-03-26'::DATE AS date;"}
                 :middleware {:format-rows? format?}}
              sw (java.io.StringWriter.)]
          (->> (qp/process-query q)
               :data :rows (csv/write-csv sw))
          [format? (str sw)]))
([true "2000,2024-03-26T00:00:00Z\n"]
 [false "2000,2024-03-26\n"])

[adam-james-v]

We already have a notion of :format-rows? and I worry that adding format-export? is adding extra knobs. Is it possible to unify these?

user=> (ns foo)
nil
foo=> (require '[clojure.data.csv :as csv]
               '[metabase.query-processor :as qp])
nil
foo=> (for [format? [true false]]
        (let [q {:database 1
                 :type     :native
                 :native   {:query "SELECT 2000 AS number, '2024-03-26'::DATE AS date;"}
                 :middleware {:format-rows? format?}}
              sw (java.io.StringWriter.)]
          (->> (qp/process-query q)
               :data :rows (csv/write-csv sw))
          [format? (str sw)]))
([true "2000,2024-03-26T00:00:00Z\n"]
 [false "2000,2024-03-26\n"])

I think that makes sense. I've changed it to unify on :format-rows. I think this will reveal a few test failures though, which might take a minute to sort out properly.

[noahmoss]

Looks reasonable to me.

[github-actions]

Codenotify: Notifying subscribers in CODENOTIFY files for diff e63d82e...da7e634.

Notify	File(s)
@kdoh	frontend/src/metabase/ui/index.ts src/metabase/email/messages.clj
@ranquild	frontend/src/metabase/sharing/components/EmailAttachmentPicker.jsx frontend/src/metabase/sharing/components/SharingSidebar/SharingSidebar.jsx

[github-actions]

@adam-james-v Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone?

[dmeremyanin]

Hey @adam-james-v, sorry for disturbing. Shouldn't the export formatting be something that needs to be turned on in the settings instead of being on by default? Or at least it would be great to turn it off in order to get back the pre-v49 behaviour globally. It's not always possible to update all the URLs that point to Metabase and using it as an API endpoint.