`metabase.util.schema` to Malli #27471

qnkhuat · 2022-12-30T12:11:11Z

Migrate metabase.util.schema to use Malli.

Rename all existing Plumatic schema to have a post fix Plumatic and the default name will be reserved for Malli.

Example:

NonBlankString -> NonBlankStringPlumatic
NonBlankString will be Mallis schema

Ideally, we could change most of defendpoint-schema to defendpoint after this. Hopefully, this is not too ambitious 🤞

Note for reviewers: this PR contains a lot of file changes but most of them are renaming.

The main change is in metabase.util.schema and test is in metabase.util.schema-test

This change is

codecov · 2022-12-30T12:24:38Z

Codecov Report

Base: 64.89% // Head: 64.96% // Increases project coverage by +0.07% 🎉

Coverage data is based on head (15d8627) compared to base (d03cc87).
Patch coverage: 88.97% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #27471      +/-   ##
==========================================
+ Coverage   64.89%   64.96%   +0.07%     
==========================================
  Files        3179     3179              
  Lines       92322    92497     +175     
  Branches    11736    11734       -2     
==========================================
+ Hits        59910    60093     +183     
+ Misses      27574    27568       -6     
+ Partials     4838     4836       -2

Flag	Coverage Δ
back-end	`85.39% <88.97%> (+0.04%)`	⬆️
front-end	`46.13% <ø> (+0.03%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...nterprise/advanced_permissions/api/application.clj	`100.00% <ø> (ø)`
...end/src/metabase_enterprise/audit_app/api/user.clj	`100.00% <ø> (ø)`
...uery_processor/middleware/handle_audit_queries.clj	`75.00% <0.00%> (ø)`
...se/backend/src/metabase_enterprise/sso/api/sso.clj	`100.00% <ø> (ø)`
src/metabase/api/activity.clj	`92.35% <ø> (ø)`
src/metabase/api/automagic_dashboards.clj	`86.52% <ø> (-0.10%)`	⬇️
src/metabase/api/google.clj	`100.00% <ø> (ø)`
src/metabase/api/login_history.clj	`33.33% <ø> (ø)`
src/metabase/api/persist.clj	`64.89% <0.00%> (ø)`
src/metabase/api/premium_features.clj	`100.00% <ø> (ø)`
... and 151 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

qnkhuat · 2022-12-30T15:08:18Z

src/metabase/util/schema.clj

@@ -399,6 +609,39 @@
                           s/Keyword                              s/Any}
    (deferred-tru "parameter must be a map with :id and :type keys")))

+(defschema ParameterSourceMalli


This is here just because I want to try to use multi.

Ideally, this should be part of ParameterMalli but I don't know how to merge a [:map] with a [:multi]. Suggestions are greatly appreciated.

this comment

qnkhuat · 2022-12-30T15:09:39Z

src/metabase/util/schema.clj

 ;; TODO - rename this to `PositiveInt`?
 (def IntGreaterThanZero
  "Schema representing an integer than must also be greater than zero."
  (with-api-error-message
    (s/constrained s/Int (partial < 0) (deferred-tru "Integer greater than zero"))
    (deferred-tru "value must be an integer greater than zero.")))

-(def NonNegativeInt


this is exactly the same as IntGreaterThanOrEqualToZero. so I replaced all the places that use it with IntGreaterThanOrEqualToZero.

qnkhuat · 2022-12-30T15:10:13Z

src/metabase/util/malli/describe.clj

@@ -185,10 +193,6 @@
 (defmethod accept 'keyword? [_ _ _ _] "keyword")
 (defmethod accept :keyword [_ _ _ _] "keyword")

-(defmethod accept 'integer? [_ _ _ _] "integer")


this was overriding the 'integer methods above.

qnkhuat · 2022-12-30T15:10:38Z

src/metabase/api/common.clj

@@ -326,6 +329,20 @@
                                               (wrap-response-if-needed
                                                (do ~@body))))))))

+(defmacro defendpoint-async-schema


write this to be asymmetry with defendpoint-schema and defendpoint

deploysentinel · 2022-12-30T15:29:14Z

No failed tests 🎉

escherize · 2022-12-30T21:15:34Z

src/metabase/util/schema.clj

+    [:map
+     [:id NonBlankStringMalli]
+     [:type keyword-or-non-blank-str-malli]
+     ;; TODO how to merge this with ParameterSourceMalli above?


best to merge these with each of the :multi's children's maps until merging multi with map is implemented in malli

ha. Thanks for letting me know.

test/metabase/util/schema_test.clj

src/metabase/util/schema.clj

escherize · 2022-12-30T22:31:50Z

What do you think of renaming the schemas in metabase.util.schema like so:

NonBlankString -> NonBlankStringPlumatic
NonBlankStringMalli -> NonBlankString

That would help highlight the fact that we want to move past the Plumatic things, and make the malli ones nicer to use.

Also, that's what I did with defendpoint.

escherize

Great work, You caught a few bugs I left in!

I had a few requests and questions but nothing major. It will be great to get more and more things using malli.

I'm not really sure if defschema is worth using, but if you keep it in, please use the Malli version of it from this comment.

Then the renaming would be nice but can be done later

qnkhuat · 2023-01-03T08:26:54Z

NonBlankString -> NonBlankStringPlumatic
NonBlankStringMalli -> NonBlankString

yes, let's do this.

I'm not really sure if defschema is worth using

yea, it was meant to make defining a schema shorter, but now I'm thinking about it doesn't save much lines of code.

schema

- NonBlankStringMalli -> NonBlankString

.clj-kondo/config.edn

…do/ignore on all existsing usage instead

…schema

escherize · 2023-01-04T17:25:20Z

src/metabase/util/schema.clj

@@ -501,9 +507,10 @@
  "Schema for a string that is a valid representation of a boolean (either `true` or `false`).
  Something that adheres to this schema is guaranteed to to work with `Boolean/parseBoolean`."
  (mc/schema
-    [:and {:error/fn (constantly (deferred-tru "value must be a valid boolean string (''true'' or ''false'')."))}
+    [:and


what do you think of using [:enum "true" "false"] here? you did setup those tests to make sure the new m.schemas are equal to the old ones, but if it's too drastic a change that makes sense too

escherize

Looks good. Just saw some failing tests having to do with s/defn being used with malli schemas. Once they're fixed we should be good to go

first step to migrate util/schema to malli

e944abb

more tests and better mesage handling

687025e

qnkhuat commented Dec 30, 2022

View reviewed changes

escherize reviewed Dec 30, 2022

View reviewed changes

test/metabase/util/schema_test.clj Show resolved Hide resolved

escherize reviewed Dec 30, 2022

View reviewed changes

src/metabase/util/schema.clj Outdated Show resolved Hide resolved

escherize reviewed Dec 30, 2022

View reviewed changes

src/metabase/util/schema.clj Outdated Show resolved Hide resolved

escherize reviewed Dec 30, 2022

View reviewed changes

src/metabase/util/schema.clj Outdated Show resolved Hide resolved

escherize requested changes Dec 30, 2022

View reviewed changes

qnkhuat added 3 commits January 3, 2023 15:54

remove with-error-message-malli and use put the error directly into the

df50f27

schema

- NonBlankString -> NonBlankStringPlumatic

fb8f29c

- NonBlankStringMalli -> NonBlankString

remove defschema and just calls mc/schema when define on

6e200f6

qnkhuat requested review from escherize and a team January 3, 2023 09:32

qnkhuat added 5 commits January 3, 2023 16:39

typo and fix kondo for defendpoint-async-schema

7aa1728

add defendpoint-schema to clj-kondo's deprecated var

818181f

enterprise namespace too

7dbed1e

fix clj-kondo failing in test

de5ba6e

still fixing kondo in test ns

e0ad6fe

camsaul reviewed Jan 4, 2023

View reviewed changes

.clj-kondo/config.edn Outdated Show resolved Hide resolved

camsaul requested a review from a team January 4, 2023 06:20

qnkhuat added 2 commits January 4, 2023 14:40

merge with master

8e3348b

remove ignoring deprecated defschema-schema in clj-kondo, use clj-kon…

6c43fa2

…do/ignore on all existsing usage instead

still miss a few places

5b56531

qnkhuat requested a review from camsaul January 4, 2023 08:21

qnkhuat added 2 commits January 4, 2023 15:23

move fn/error to fn instead of leaving it it the outer :and for some …

98f7898

…schema

Merge branch 'master' into ngoc-malli-for-util-schema

3830108

escherize reviewed Jan 4, 2023

View reviewed changes

camsaul removed their request for review January 4, 2023 17:51

qnkhuat requested a review from a team January 5, 2023 06:59

qnkhuat added 2 commits January 5, 2023 14:21

merge with master and fix conflicts

144aea9

white space fix. *insert same same but different meme*

8aa22b9

qnkhuat requested a review from escherize January 5, 2023 07:31

clj-kondo warning fixes

8b44d4b

escherize approved these changes Jan 5, 2023

View reviewed changes

fix misalign plumatic schema

15d8627

qnkhuat enabled auto-merge (squash) January 5, 2023 12:07

qnkhuat merged commit 6d7f778 into master Jan 5, 2023

qnkhuat deleted the ngoc-malli-for-util-schema branch January 5, 2023 13:28

dpsutton mentioned this pull request Jan 10, 2023

Undo #27471 and make an analogous malli util namespace #27618

Closed

darksciencebase added the .Team/Backend 🥞 label Mar 22, 2023

alxnddr added the .Team/QuestionMasters label May 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`metabase.util.schema` to Malli #27471

`metabase.util.schema` to Malli #27471

qnkhuat commented Dec 30, 2022 •

edited by darksciencebase

codecov bot commented Dec 30, 2022 •

edited

qnkhuat Dec 30, 2022

escherize Dec 30, 2022

qnkhuat Dec 30, 2022

qnkhuat Dec 30, 2022

qnkhuat Dec 30, 2022

escherize Dec 30, 2022

deploysentinel bot commented Dec 30, 2022 •

edited

escherize Dec 30, 2022 •

edited

qnkhuat Jan 3, 2023

escherize commented Dec 30, 2022 •

edited

escherize left a comment •

edited

qnkhuat commented Jan 3, 2023 •

edited

escherize Jan 4, 2023

escherize left a comment •

edited

metabase.util.schema to Malli #27471

metabase.util.schema to Malli #27471

Conversation

qnkhuat commented Dec 30, 2022 • edited by darksciencebase

codecov bot commented Dec 30, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deploysentinel bot commented Dec 30, 2022 • edited

escherize Dec 30, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

escherize commented Dec 30, 2022 • edited

escherize left a comment • edited

Choose a reason for hiding this comment

qnkhuat commented Jan 3, 2023 • edited

Choose a reason for hiding this comment

escherize left a comment • edited

Choose a reason for hiding this comment

`metabase.util.schema` to Malli #27471

`metabase.util.schema` to Malli #27471

qnkhuat commented Dec 30, 2022 •

edited by darksciencebase

codecov bot commented Dec 30, 2022 •

edited

deploysentinel bot commented Dec 30, 2022 •

edited

escherize Dec 30, 2022 •

edited

escherize commented Dec 30, 2022 •

edited

escherize left a comment •

edited

qnkhuat commented Jan 3, 2023 •

edited

escherize left a comment •

edited