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

DataTemplate.readRecord broken on newly generated scala records #72

Open
eboto opened this issue Jan 16, 2018 · 2 comments
Open

DataTemplate.readRecord broken on newly generated scala records #72

eboto opened this issue Jan 16, 2018 · 2 comments

Comments

@eboto
Copy link
Contributor

eboto commented Jan 16, 2018

(See #71 for reproduction)

For quite a few versions now (since 5acb817) I think generated scala records have been incompatible with the readRecord methods in org.coursera.courier.templates.DataTemplates

Attempting to read a record results in java.lang.NoSuchMethodException: org.coursera.records.test.Simple$.apply, of course because the previous apply(DataMap, DataConversion) is now build(DataMap, DataConversion)

My temptation is to update DataTemplates to call build instead of apply, but that would be backwards incompatible for Courier users who are still operating on old templates. Could also perform two lookups in case the first one fails, but that will double the reflection work for either old or new clients. Curious your thoughts how to remediate?

This is relatively high priority for us at Instrumental so would love your thoughts.

@eboto eboto changed the title DataTemplate.readRecord broken on newly generated definitions DataTemplate.readRecord broken on newly generated scala records Jan 16, 2018
eboto pushed a commit to eboto/courier that referenced this issue Jan 16, 2018
@eboto
Copy link
Contributor Author

eboto commented Jan 17, 2018

OK the attached pull request fixes the issue. The new behavior is:

  • First attempt to look up the "build" method on the template companion
  • If absent, secondly attempt the "apply" method.

This should fix the problem with identical performance for most recently generated templates, and will do the work slightly slower for older templates without breaking backwards compatibility.

Not sure who is maintainer now. @zhaojunz @amory-coursera but would appreciate a review merge and version on this one assuming passing CI (looks like travis has a pretty big queue right now)

@eboto
Copy link
Contributor Author

eboto commented Jan 23, 2018

@yifan-coursera @amory-coursera any chance you could point the maintainer for this project to take a look at this issue and PR? Seems high-priority -- I don't see how DataTemplates.readX isn't completely broken for anyone using recent courier templates right now!

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

No branches or pull requests

1 participant