Skip to content

Conversation

@eft-prima
Copy link
Contributor

@eft-prima eft-prima commented Nov 6, 2025

https://prima-assicurazioni-spa.myjetbrains.com/youtrack/issue/PMPOSU-777

This pull request fixes a bug in the Avrogen.Schema.external_dependencies function, ensuring that previously defined types within a schema are no longer incorrectly identified as external dependencies. The changes include refactoring the implementation to properly track defined types, updating tests to cover these cases, and bumping the package version.

Bug fix and implementation improvements

  • Refactored the external_dependencies function in lib/avrogen/schema.ex to track previously defined types and exclude them from external dependencies. This includes introducing the handle_field helper and correctly handling unions and named types.

Testing enhancements

  • Added new test cases in test/schema_test.exs to verify that previously defined types (with and without namespace) are not identified as external dependencies.

Documentation and release

  • Updated the CHANGELOG.md to document the bug fix for external dependencies detection.
  • Bumped the version in mix.exs from 0.9.0 to 0.10.0 to reflect the new release.

@eft-prima eft-prima marked this pull request as ready for review November 6, 2025 18:25
@eft-prima eft-prima requested a review from a team as a code owner November 6, 2025 18:25
EddieWhi
EddieWhi previously approved these changes Nov 7, 2025
@EddieWhi
Copy link

EddieWhi commented Nov 7, 2025

Slack conversation for prosperity:
Eddie Whiteside
Nice, thanks Ed 🙂
What I can't work out from the PR is how it worked previously with the more nested types???

Ed Fawcett-Taylor
Ah, so that's because Avrogen.Schema.external_dependencies never looked at more nested types, it just looked at the types of the fields in a top level "record" type. The resolving of more nested types is done by Avrogen.Avro.Schema.generate_code , which doesn't have the same issue.
This PR fixes my immediate problem but it wouldn't handle a situation where there's a deeply nested reference to an actual external type.

Eddie Whiteside
Oh... it didn't recurse atall? So you can't have an external type at a more nested level.
Right!
I see.

Ed Fawcett-Taylor
Yeah, still work to be done but this was the smallest change I could do that would unblock me!

Eddie Whiteside
For sure.

cpiemontese
cpiemontese previously approved these changes Nov 7, 2025
Copy link
Contributor

@cpiemontese cpiemontese left a comment

Choose a reason for hiding this comment

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

+1 from me, if you run suite-py bump you can also update the CHANGELOG file (or you can update it manually) so we keep everything aligned and then I'll release

@eft-prima eft-prima dismissed stale reviews from cpiemontese and EddieWhi via 6f85eaf November 7, 2025 10:24
@eft-prima
Copy link
Contributor Author

+1 from me, if you run suite-py bump you can also update the CHANGELOG file (or you can update it manually) so we keep everything aligned and then I'll release

Oh cool, I didn't know it did that! Done.

@eft-prima eft-prima requested a review from cpiemontese November 7, 2025 10:24
@cpiemontese cpiemontese merged commit faa667c into master Nov 7, 2025
9 checks passed
@cpiemontese cpiemontese deleted the PMPOSU-777/task/avrogen-handle-references-to-previous-defined-schema-in-top-level-record-types branch November 7, 2025 10:25
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.

3 participants