Skip to content

remove check for ash embedded resource (allow all types in attributes) - #1

Merged
futhr merged 5 commits into
futhr:mainfrom
HaimKortovich:fix-typed-structs-in-attributes
Jun 10, 2026
Merged

remove check for ash embedded resource (allow all types in attributes)#1
futhr merged 5 commits into
futhr:mainfrom
HaimKortovich:fix-typed-structs-in-attributes

Conversation

@HaimKortovich

@HaimKortovich HaimKortovich commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

when using typed structs inside an attribute it defaulted to string (see tests)

Summary by Sourcery

Relax embedded resource detection to support non-Ash.Resource types (e.g. typed structs) in attributes and extend the kitchen sink examples to cover discriminated union actors.

Enhancements:

  • Allow any loaded Spark DSL module (not only embedded Ash resources) to be treated as an embedded resource type for schema generation.

Tests:

  • Add typed struct Person and Company variants and an Actor union type with a discriminator, and cover them via new kitchen sink tests and attributes to reproduce and verify correct schema generation for discriminated unions.

Greptile Summary

This PR fixes Ash.TypedStruct attributes silently degrading to "type": "string" in generated OpenAPI specs. The root cause was that normalize_newtype/1 (formerly inlined) passed the resolved subtype (Ash.Type.Struct) through normalize_type/1, which had no mapping for it and fell through to :string.

  • Introduces normalize_newtype/1 which detects Ash.Type.Struct subtypes and routes them to the new build_typed_struct_schema/1, which reads subtype_constraints()[:fields] to produce a typed object schema with accurate properties and a required list.
  • Reorders normalize_complex_type/1 so json_schema/1 callbacks take priority over the embedded-resource check, aligning the priority with explicit user intent.
  • Adds Person, Company, and Actor test fixtures and covers both the discriminated-union (anyOf) and direct-attribute (inline object) paths across OpenAPI 3.0 and 3.1, including Oaskit end-to-end validation.

Confidence Score: 5/5

Safe to merge — the change is additive, covers a previously broken code path, and is validated end-to-end by Oaskit.

The core logic in normalize_newtype and build_typed_struct_schema is correct: it reads subtype_constraints()[:fields], maps declared field types through the existing ash_type_to_base_schema pipeline, and sorts the required list. The priority reorder (json_schema callback before embedded check) is intentional and correct. No breaking changes to existing embedded-resource or union paths. The new Oaskit validation tests confirm the generated spec is structurally valid for both 3.0 and 3.1.

No files require special attention beyond the documentation gaps noted in the comments.

Important Files Changed

Filename Overview
lib/ash_oaskit/core/type_mapper.ex Adds normalize_newtype/1 to route Ash.Type.Struct NewTypes to a new build_typed_struct_schema/1 path; reorders embedded vs json_schema priority. Logic is sound, but lacks unit tests in type_mapper_test.exs and external doc updates (README, usage-rules).
test/support/kitchen_sink_resources.ex Adds Person, Company TypedStruct modules and an Actor discriminated-union NewType; attaches them as actor and owner attributes on KitchenSink. Well-structured regression fixtures.
test/ash_oaskit/kitchen_sink_test.exs Adds integration tests for the new typed-struct and discriminated-union paths, plus Oaskit end-to-end validation for both 3.0 and 3.1. Tests correctly use inline schema assertions (no broken $ref expectations).

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["normalize_complex_type(type)"] --> B{"has_json_schema_callback?"}
    B -- yes --> C["{:custom, json_schema}"]
    B -- no --> D{"embedded_resource?"}
    D -- yes --> E["{:embedded, type} → $ref"]
    D -- no --> F{"get_union_types?"}
    F -- yes --> G["{:union, types} → anyOf"]
    F -- no --> H{"enum_type?"}
    H -- yes --> I["{:custom, enum_schema}"]
    H -- no --> J{"newtype?"}
    J -- no --> K[":string fallback"]
    J -- yes --> L["normalize_newtype(type)"]
    L --> M{"subtype_of == Ash.Type.Struct?"}
    M -- yes --> N["{:struct_fields, type}"]
    N --> O["build_typed_struct_schema\nreads subtype_constraints()[:fields]\nproduces inline object schema"]
    M -- no --> P["normalize_type(subtype)\nrecurse on resolved subtype"]

    style N fill:#6bcb77,color:#000
    style O fill:#6bcb77,color:#000
    style L fill:#6bcb77,color:#000
    style M fill:#6bcb77,color:#000
Loading

Reviews (4): Last reviewed commit: "Merge branch 'main' into fix-typed-struc..." | Re-trigger Greptile

Context used:

  • Context used - AGENTS.md (source)

@sourcery-ai

sourcery-ai Bot commented Jun 1, 2026

Copy link
Copy Markdown

Reviewer's Guide

This PR fixes incorrect handling of typed structs used as attribute types by relaxing the embedded resource check, and adds a discriminated union example (Actor) with associated tests to ensure correct OpenAPI schema generation.

File-Level Changes

Change Details Files
Add discriminated union example using typed structs to reproduce and guard the bug
  • Introduce typed structs Person and Company with required and optional fields using Ash.TypedStruct
  • Define Actor as a union new-type with discriminator-based constraints over Person and Company variants
  • Add an :actor attribute to the KitchenSink resource using the new Actor union type with documentation
test/support/kitchen_sink_resources.ex
Extend KitchenSink tests to cover discriminated union schema generation
  • Add test group for the custom discriminated union tagged actor attribute
  • Assert that the generated OpenAPI schema for actor uses anyOf with null and $ref entries for Person and Company schemas, with appropriate titles and description
test/ash_oaskit/kitchen_sink_test.exs
Relax embedded resource detection so all valid Ash types (including typed structs) are allowed for attributes
  • Change embedded_resource?/1 to only require the module to be loaded and export spark_is/0, removing the Ash.Resource and embedded checks
  • Delete the ash_embedded?/1 helper and its use, simplifying the resource type predicate
lib/ash_oaskit/core/type_mapper.ex

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hey - I've left some high level feedback:

  • The embedded_resource?/1 helper now only checks for spark_is/0, which means it will treat any Spark DSL module (not just embedded Ash resources) as an embedded resource; either narrow the predicate (e.g., keep the Spark.Dsl.is?(type, Ash.Resource) check) or rename the function to reflect its broader semantics.
  • With the new behavior in embedded_resource?/1, consider whether non-embedded resources or other DSL modules should now be handled differently downstream (e.g., when generating schemas) to avoid unexpected $ref generation for types that are not actually embedded resources.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The `embedded_resource?/1` helper now only checks for `spark_is/0`, which means it will treat any Spark DSL module (not just embedded Ash resources) as an embedded resource; either narrow the predicate (e.g., keep the `Spark.Dsl.is?(type, Ash.Resource)` check) or rename the function to reflect its broader semantics.
- With the new behavior in `embedded_resource?/1`, consider whether non-embedded resources or other DSL modules should now be handled differently downstream (e.g., when generating schemas) to avoid unexpected `$ref` generation for types that are not actually embedded resources.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread lib/ash_oaskit/core/type_mapper.ex
Comment thread test/ash_oaskit/kitchen_sink_test.exs Outdated
Comment thread test/support/kitchen_sink_resources.ex Outdated
HaimKortovich and others added 4 commits June 1, 2026 10:43
Resolves the type_mapper conflict on top of the 0.2.0 rewrite:

- keep the json_schema/1-callback-before-embedded reorder from this
  branch
- detect typed structs precisely as NewTypes of Ash.Type.Struct
  (instead of the spark_is/0 check, which matches any Spark DSL
  module) and build object schemas from the NewType's field
  constraints, so declared field types are kept (integer stays
  integer) and allow_nil? false fields become required
- mark the actor fixture public? true per the 0.2.0 visibility rules,
  add a direct typed-struct attribute (owner), and extend the tests:
  corrected variant assertions, 3.0 coverage, and Oaskit validation
  of the Lab domain spec in both versions
futhr added a commit that referenced this pull request Jun 10, 2026
setup-beam resolves "28" to the latest patch build, and a PLT created
on an older patch (28.5.0.1) fails with "Old PLT file" once a newer
one (28.5.0.2) ships — exactly what broke the dialyzer job on PR #1.
Keying on the resolved versions makes a toolchain bump rebuild the PLT
instead of failing.
@futhr

futhr commented Jun 10, 2026

Copy link
Copy Markdown
Owner

Thanks for digging into this and for the clean repro with the Person/Company union, that made the problem easy to verify. You were right that typed structs fell through to a plain string schema, and that was still the case after the 0.2.0 rewrite.

A few things changed while landing it on top of 0.2.0:

  • the spark_is/0 check matched any Spark DSL module (domains, full resources), so I swapped it for detecting NewTypes of Ash.Type.Struct directly, which is exactly what Ash.TypedStruct compiles to
  • struct fields now keep their declared types instead of all mapping to string, so age and employee_count come out as integer, and allow_nil?: false fields end up in required
  • the actor fixture needed public? true since 0.2.0 only documents public fields, and I added a direct typed-struct attribute plus 3.0 and validation coverage on top of your tests

Appreciate the contribution.

@futhr
futhr merged commit 48cab28 into futhr:main Jun 10, 2026
13 checks passed
futhr added a commit that referenced this pull request Jun 10, 2026
setup-beam resolves "28" to the latest patch build, and a PLT created
on an older patch (28.5.0.1) fails with "Old PLT file" once a newer
one (28.5.0.2) ships — exactly what broke the dialyzer job on PR #1.
Keying on the resolved versions makes a toolchain bump rebuild the PLT
instead of failing.
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