Skip to content
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

Setting guardrails registry as default malli registry doesn't work #43

Open
RokLenarcic opened this issue Mar 18, 2024 · 8 comments
Open

Comments

@RokLenarcic
Copy link
Contributor

(require '[com.fulcrologic.guardrails.malli.registry :as gr.reg])
=> nil
(malli.registry/set-default-registry! gr.reg/registry)
=>
#object[malli.registry$composite_registry$reify__3394
        0x626f9de6
        "malli.registry$composite_registry$reify__3394@626f9de6"]
(malli.core/schema [:map [:x :string]])
Execution error (StackOverflowError) at malli.registry$custom_default_registry$reify__3390/_schema (registry.cljc:48).
null

The reason for this is simple, guardrails registry refers to default registry and when it becomes default registry, that's a loop.

Why would one want to set guardrails as default registry? For consistency. Using >def or register! fills up the guardrails registry and if you want to use those schemas in non-guardrails code you'll want to use that registry. E.g. malli.util/optional-keys. Of course you can always pick registry on per-call/per-schema basis, so that fixes this, but it's kinda annoying.

@ikitommi
Copy link
Contributor

Guardrails registry uses malli registry so this is possible:

  1. create a mutable default registry into Malli
  2. register schemas there
  3. ... they are automatically available for guardrails too

what is the use case to inject those guardrails schemas back to malli registry?

@RokLenarcic
Copy link
Contributor Author

Right, now you are talking about the other way around: yes, the way it's set up is that Guardrails uses registry that automatically sees everything from Malli default registry.

But I am talking about the common use case, that is, the person is using Guardrails primarily and doesn't touch Malli directly unless needed. So the person is using Guardrails >def and register! to add schemas. These schemas work with all the Guardrails stuff.

But then you try to use malli.util/optional-keys, or some other helper function, it will use Malli default registry, which has no idea about all the schemas that you registered with >def, so you get an error. At this point I tried setting Guardrails registry as malli default registry, which would solve that, but then I get StackOverflowError.

Now, I've solved this by making a new utility function:

(defn optional-keys [?schema ?keys] (mu/optional-keys ?schema ?keys {:registry gr.reg/registry}))

So I guess this is fixed, I just have to use and curate a set of functions like this.

@RokLenarcic
Copy link
Contributor Author

I guess my issue is using malli.util/merge and malli.util/optional-keys with Guardrails. Besides the above mentioned problem with registries, I've now ran into another problem: when guardrails is disabled, the >def schemas are absent but my calls to malli.util/merge expect to find them and it results in an error.

@RokLenarcic RokLenarcic reopened this Mar 22, 2024
@RokLenarcic
Copy link
Contributor Author

@ikitommi
I guess a problem I am also having here is that instead of registering schemas I like to use them as data, if they are not used in tons of different namespaces, e.g.:

(def Dividend
  (SecurityWith
    [:map
     [::id :str+]
     ::mod/ts
     [::amount :+amount]
     [::tax {:optional true} :+amount]
     [::relief-statement {:optional true} :str+]
     [::description :string]
     [::type :string]]))
     
(def DividendOrReversal
  ;; reversal has negative amount
  (mod/sch-merge Dividend [:map [::amount :amount]]))

(def DividendSeq [:sequential Dividend])

I like using that var as a value in other schemas etc... and this kinda breaks with Guardrails model since these always get compiled whether it's enabled or not, and they will try to resolve their dependants when used with merge and similar.

@awkay
Copy link
Member

awkay commented Apr 16, 2024

I'd be interested in discussing your experiences further. The aim is to make it so that the GR stuff can be added to a project without affecting the default registry (breaking existing programs), but that you can use what's in the default registry in GR.

Do you have any specific proposals for change that meet all of the current design goals and won't break existing code?

@RokLenarcic
Copy link
Contributor Author

Your goal to have GR be drop-in for existing projects using Malli has obstacles:

  1. Malli utility functions

These functions are very useful but they will eagerly evaluate schemas, and try to resolve them from default registry, which will throw exceptions for schemas defined by >def, as they are not in the default registry.

The ones I use a lot are: malli.util/merge and malli.util/optional-keys. All of these calls have to be changed to specify GR registry, I just write my own wrapper:

(defn sch-merge [x y] (mu/merge x y {:registry gr.reg/registry}))

This change is trivial to do in the codebase, but it is a change.

  1. Conditionally defining vars

This was subject of another issue I opened. It is common, and suggested by Malli documentation, that you define chunks of schemas as vars or perhaps functions that generate them. It's all just data anyway, right? See SecurityWith function above, I have a bunch of similar schemas, I define a function to generate them.

The issue there is again that some of those use malli.util functions that resolve schemas. Ok, I've patched it to use GR registry, but it's not good enough. If any of schemas defined by a def uses a schema resolving function AND references a schema defined by a >def it will break when GR is turned off (production). Observe:

(>def ::currency [:string {:min 3 :max 3}])
(>def ::ts :time/local-date-time)
(>def ::amount [:tuple decimal? ::currency])

(>def ::transfer [:map ::amount ::ts])
(>def ::owned [:map [:owner :string]])

(def OwnedTransfer (mu/merge ::transfer ::owned {:registry gr.reg/registry}))

This will error out with GR disabled, work with GR enabled. So here the user has a couple of options:

  • change def OwnedTransfer to >def ::owned-transfer and then change all var usages to use keyword if possible
  • change def OwnedTransfer to defn OwnedTransfer and then change all the var usages to 0-arg function calls
  • changing def OwnedTranser to >def OwnedTransfer is not supported

So you see it's not as easy as just changing defs to >defs as that is not supported for vars. In my PR I just added ability to act as a def for vars to >def. If that's not a good solution, then at least a general macro facility is needed that says: evaluate this if GR is enabled, otherwise compile to nil. Then I might solve this myself:

(def OwnedTransfer (gr/> (mu/merge ::transfer ::owned {:registry gr.reg/registry}))

@awkay
Copy link
Member

awkay commented Jun 3, 2024

So, you can use GR's config ns to read if GR is enabled at compile time. This has been the biggest annoyance in my own use of Malli and GR as well. I've been hitting cases where I've had to copy code out of Malli in order to get it to even let me say which registry to use. I have sort of left this as "an exercise for the reader" because I myself don't see me being able to provide a one-size-fits-all solution to the problems.

Perhaps the best solution I can provide is some kind of config that lets you use the default registry instead of the one in GR?

@RokLenarcic
Copy link
Contributor Author

Hm if I tell GR to use the default registry, how will GR add schemas to the registry? I don't think there's an API for that, the registry itself is opaque and the person who sets the default registry to be some sort of mutable registry, also has to know the mechanism how to add to it. I don't think there's an abstract way to do it. GR cannot add schemas to the default registry with no regard for the implementation type.

That's why my first idea was to make GR registry the default registry (since GR knows how to add entries to it). But that got prevented by GR logic of its registry delegating (also) to the default registry, which caused a loop in that case (GR registry was delegating to itself, causing a stack overflow). This mechanism also prevents the default registry being any sort of compound registry where one subregistry is the GR one, you'll get a loop again.

You've stated that your main goal here is to have it be usable as a dropin without breaking apps. The main obstacle, as explained above, is that in Malli chunks of schemas are often declared/created as just a datastructure without any registering. See examples above, having a def or a defn that returns a schema. Just converting these to >def doesn't work out of the box because >def ASymbol doesn't work.

Anyway I've described different possible approaches. Rolling my own macros is not very appealing because they'll be missing from the noop namespace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants