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

Allow extra avsc file includes #36

Merged
merged 5 commits into from
Sep 16, 2020

Conversation

ojung
Copy link
Contributor

@ojung ojung commented Sep 8, 2020

Hey 👋

We have a use case where we want to import 3rd party avsc files into our schemas to be able to reference the types defined inside. Since the avro library dependencies (avroUnpackDependencies / target) were compiled independently of the avroSource files, this was not possible.

This PR allows specifying additional avroIncludes and adds avroUnpackDependencies / target by default.

The behavior (apart from the default of avroUnpackDependencies / target) is similar to the avro-maven-plugin, where you can specify extra imports explicitly.

Copy link
Collaborator

@nevillelyh nevillelyh left a comment

Choose a reason for hiding this comment

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

LGTM, @RustedBones any thoughts?

Copy link
Collaborator

@RustedBones RustedBones left a comment

Choose a reason for hiding this comment

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

I feel the issue is more due to the usage of 2 separate parsers in the compileAvroSchema steps. Reusing the instance will grant access to the external types in the module.

I wanted 1st to merge the 2 compileAvroSchema into one single step but the custom avroUseNamespace made it hard to achieve. I think we should prefer this way though.

compileAvroSchema(externalSrcDir, outDir, out.log, strType, fieldVis, enbDecimal, useNs, builder)
compileAvroSchema(srcDir, outDir, out.log, strType, fieldVis, enbDecimal, useNs, builder)
compileAvroSchema(externalSrcDir, outDir, Seq.empty, out.log, strType, fieldVis, enbDecimal, useNs, builder)
compileAvroSchema(srcDir, outDir, includes, out.log, strType, fieldVis, enbDecimal, useNs, builder)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Won 't we compile the include - avroUnpackDependencies / target twice with this ?

Copy link
Contributor Author

@ojung ojung Sep 11, 2020

Choose a reason for hiding this comment

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

Yes, you're right. See #36 (comment) for a suggestion on how to avoid having this problem by default.

Copy link
Collaborator

@RustedBones RustedBones Sep 16, 2020

Choose a reason for hiding this comment

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

The cacheCompile below needs to be updated with the includes folder

@ojung
Copy link
Contributor Author

ojung commented Sep 11, 2020

I feel the issue is more due to the usage of 2 separate parsers in the compileAvroSchema steps. Reusing the instance will grant access to the external types in the module.

I wanted 1st to merge the 2 compileAvroSchema into one single step but the custom avroUseNamespace made it hard to achieve. I think we should prefer this way though.

I went with the additional includes approach because

  1. I know it from the avro-maven-plugin and since it is an official apache avro plugin I supposed it would be best to stay as close as possible to it's behavior.
  2. I didn't want to break backward compatibility by removing the avroUseNamespace option.

The ability to extract avsc files from libraryDependencies can be considered independent of the ability to include extra avro files. I.e. you don't have to avroInclude your libraryDependencies and also, you can avroInclude extra avsc files that are not extracted from your libraryDependencies.

I think to make this change as non-disruptive as possible I would rather simply not add avroUnpackDependencies / target by default to the avroIncludes. It can still be added explicitly by users of the plugin.

Wdyt?

@ojung ojung force-pushed the add-avsc-file-includes branch from 84d5181 to ec8312e Compare September 14, 2020 13:28
@ojung ojung force-pushed the add-avsc-file-includes branch from ec8312e to 1c0f4a7 Compare September 14, 2020 13:33
@ojung
Copy link
Contributor Author

ojung commented Sep 14, 2020

Hey @RustedBones, I have updated the PR. Can you have another look?

Copy link
Collaborator

@RustedBones RustedBones left a comment

Choose a reason for hiding this comment

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

Have a look to flix-tech#2.
I tried to factorize code so only one schema builder is used for all src / unpacked / includes folder

@ojung
Copy link
Contributor Author

ojung commented Sep 16, 2020

Looks great :) Shall I cherry-pick your commit onto this PR?

@RustedBones
Copy link
Collaborator

@ojung you can merge the PR into your branch, it will update here.
I'll squash merge the commits into sbt-avro master branch in the end so no worries.

@ojung ojung force-pushed the add-avsc-file-includes branch from b8b2cb9 to 1c0f4a7 Compare September 16, 2020 15:01
@ojung
Copy link
Contributor Author

ojung commented Sep 16, 2020

Ok, merged in your commit.

@RustedBones RustedBones merged commit 8e129ca into sbt:master Sep 16, 2020
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