Skip to content

ProductAsArray attribute added, alternative rendering of SProduct#3941

Merged
adamw merged 5 commits into
softwaremill:masterfrom
zorba128:master
Jul 25, 2024
Merged

ProductAsArray attribute added, alternative rendering of SProduct#3941
adamw merged 5 commits into
softwaremill:masterfrom
zorba128:master

Conversation

@zorba128

Copy link
Copy Markdown
Contributor

Relates to #2604.

Looking at https://json-schema.org/understanding-json-schema/reference/array#tupleValidation it seems Array is natural representation for tuples (and this is what circe mentioned in the ticket does by default).

  • For now PR only adds attribute to allow such schema to be defined; if agreed - probably default schema generation for scala tuples should be updated to enable it by default.
  • Also schema rendering should be updated, as it produces Draft04, which used slightly different way of representing array element type constraints (items instead of prefixItems).
  • array arity constraint should be also rendered

…r than Object.

Signed-off-by: Marcin Kielar <zorba128@interia.pl>
@zorba128

Copy link
Copy Markdown
Contributor Author

Alternative solution would be to introduce STuple schema type, that would directly model tuples (products with fields accessible by index rather than name). Then depending on needs it could get rendered to object (for compatibility if neeed) or array schema.

Or extend SArray to allow specification of elements by index.

But both solutions seem like bigger undertakings compared to proposed in PR.

case TSchemaType.SBoolean() => ASchema(SchemaType.Boolean)
case TSchemaType.SString() => ASchema(SchemaType.String)
case TSchemaType.SProduct(fields) if schema.attribute(TSchema.ProductAsArray.Attribute).map(_.productAsArray).getOrElse(false) =>
// TODO: Draft04 uses alternative form of `items` instead

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Draft04 is only used here:

So maybe let's change it to 2020-12? That's what used by apispec

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think already output of TapirSchemaToJsonSchema looks more like 2020-12 than Draft04. Few tests show schemas validate correctly (against meta schema). Can I push this change to this PR?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes please!

val Attribute: AttributeKey[UniqueItems] = new AttributeKey[UniqueItems]("sttp.tapir.Schema.UniqueItems")
}

case class ProductAsArray(productAsArray: Boolean)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we could add a comment here, explaining what is the purpose of this attribute, or at least linking to this issue number (#3941)?

@adamw

adamw commented Jul 19, 2024

Copy link
Copy Markdown
Member

Looks good, and I agree that's currently a better solution that is much less disruptive then trying to add a new SchemaType.

Shouldn't we also add implicit schemas for tuples? Having their schemas generated as case classes is probably not what anybody wants.

zorba128 added 2 commits July 20, 2024 10:03
Signed-off-by: Marcin Kielar <zorba128@interia.pl>
Signed-off-by: Marcin Kielar <zorba128@interia.pl>
@zorba128

Copy link
Copy Markdown
Contributor Author

Shouldn't we also add implicit schemas for tuples? Having their schemas generated as case classes is probably not what anybody wants.

Definitely - maybe someone more familiar with tapir's schema generation macros could take a look? It would take a while to dig into it...

zorba128 and others added 2 commits July 23, 2024 09:01
Signed-off-by: Marcin Kielar <zorba128@interia.pl>
@adamw
adamw merged commit 57bb396 into softwaremill:master Jul 25, 2024
@adamw

adamw commented Jul 25, 2024

Copy link
Copy Markdown
Member

Thanks, I'll add the schemas in a separate PR

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

Successfully merging this pull request may close these issues.

2 participants