coursera / courier

Data interchange for the modern web + mobile stack.
http://coursera.github.io/courier/
Apache License 2.0
98 stars 24 forks source link

DataTemplate.readRecord broken on newly generated scala records #72

Open eboto opened 6 years ago

eboto commented 6 years ago

(See #71 for reproduction)

For quite a few versions now (since 5acb817876e5109d9acb640f3c4f87f47a8f4c83) 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 commented 6 years ago

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

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 commented 6 years ago

@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!