cognitect-labs / vase

Data driven microservices
Eclipse Public License 1.0
375 stars 41 forks source link

Schema-tx literal surprising behavior when :db/doc omitted #44

Closed mtnygard closed 7 years ago

mtnygard commented 7 years ago

Description

Expected Behavior

Consider the following sample:

[:pet/legs :ref :many]

This is not accepted and throws an exception because the docstring is missing. This is reasonable if the docstring is required. However:

[:pet/legs :ref :many :indexed]

This also fails but silently. The :indexed toggle is misinterpreted as a docstring. It has to be changed to:

[:pet/legs :ref :many :indexed "Why would you search by leg number? And which one is #1?"]

This arises because the toggles are optional and may be multiple.

We should go one of two ways on this. Either:

  1. Make the docstring required in all cases and throw when it is not a string, or
  2. Make the docstring optional in all cases and accept as many toggles as we find.

I lean toward #2, since the whole point of the schema-tx literal is convenience and concision.

Pedestal and Vase version

Present in 0.9.1-snapshot

ohpauleez commented 7 years ago

Since the very first version of Vase, many users have asked for the docstrings to be optional. I always rejected the change, but let's use this issue to discuss the trade-offs.

Why Vase enforces schema docstrings

In your example above, based on "many ref", I assumed it was a reference to all of the leg entities (maybe they carry their own information; Maybe you can build/configure a pet piecewise), but the docstring example implies that the value should be a ref to a number... it's actually leg count. Without a docstring, I'd have to make an assumption, find out based on use within the service, and hold the outcome within my head -- Without a docstring, I incur unnecessary cognitive load.

Are docstrings annoying to type? Sure, sometimes. Is it something we should do anyway? I believe so. Are we better off in the long run with docstrings present? I believe so.

Arguments against docstrings

mtnygard commented 7 years ago

I'm OK with requiring docstrings. We should enforce that they are actually strings, though, and not just eating the last keyword option in the vector.

mtnygard commented 7 years ago

I found that we had no tests for #vase/schema-tx, so I'm adding some.

The code for opt-toggles currently only accepts one of #{:unique :index :fulltext :identity :component}. I believe this is incorrect. Those options are not mutually exclusive and there are valid use cases for their combinations. I'm going to write the test to allow multiple toggles. I'm also adding :no-history.