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

Add support for runtime analysis at Java 6 #66

Closed
varahash opened this issue Jul 9, 2018 · 17 comments
Closed

Add support for runtime analysis at Java 6 #66

varahash opened this issue Jul 9, 2018 · 17 comments

Comments

@varahash
Copy link
Contributor

varahash commented Jul 9, 2018

This will allow use reflection data binding on platform which currently does not support Java8 (e.g. Android prior API 24).

I propose to move appropriate classes from 'dsl-json-java8' module to core module 'dsl-json' or we can create new one 'dsl-json-reflection'.

It seems relatively easy to do. We just need to replace usage of 'java.lang.function.*' with our custom interfaces. And keep ImmutableAnalyzer class in 'dsl-json-java8' module, since parameter names are not supported prior Java8.

@zapov
Copy link
Member

zapov commented Jul 9, 2018

I thought about this when porting the processor and while I don't mind moving stuff around I put it all in Java8, mostly to simplify number of dependencies and setting up the project.
One could argue that Java8 processor and reflection should be moved to dsl-json project (which is totally valid argument).

I don't think reflection part of dsl-json is all that useful, since it supports much less stuff than annotation processor (it doesn't do private reflection overrides and similar stuff which it could).

So if we want to support Java6 we should probably move annotation processor to Java6 compatible project (even maybe core project).

@varahash
Copy link
Contributor Author

varahash commented Jul 9, 2018

I know at least one issue with generated code. If POJO contains property with type 'List', the generated code tries find reader/writer for List type, but it fails in case CollectionAnalyzer not registered.

@varahash
Copy link
Contributor Author

varahash commented Jul 9, 2018

Do we realy need move annotation processor to Java6? When developing project we can use Java8 compiler to compile java6 project.

@varahash
Copy link
Contributor Author

varahash commented Jul 9, 2018

Or means that generated code should be Java6?

@zapov
Copy link
Member

zapov commented Jul 9, 2018

I meant the gen-code should be Java6. But there are some deps which ought to be moved to dsl-json.
Shouldn't old Android cope with Java8 project as long as there are no lambda and other Java8 features used?

@varahash
Copy link
Contributor Author

varahash commented Jul 9, 2018

Yes, currently android build tools can convert(aka 'desugar') java8 code (with labdas) to java6, but if Java8 classes used like ' java.lang.function.*' then app wiil crash at runtime.
In any way generated code already java6 compatible, except case when 'dsljson.inline=false' option used.

@zapov
Copy link
Member

zapov commented Jul 9, 2018

So what do we need to change except create our own signature instead of function?

I'm keen on dropping that inline=false thing. It doesn't seem as useful anymore (and it's not really properly tested)

@varahash
Copy link
Contributor Author

varahash commented Jul 9, 2018

It seems rest of code is compatible.

@zapov
Copy link
Member

zapov commented Jul 9, 2018

Let's change the problematic signatures then

@varahash
Copy link
Contributor Author

There is problem, since 'dsl-json-java8' module registers service configuration 'ConfigureJava8' which uses java8 specific classes (Optional*, java.time.*), so we will get runtime exception.

@zapov
Copy link
Member

zapov commented Jul 11, 2018

Indeed. I guess the best solution seems to move compile time deps to dsl-json and allow usage of java8 project as APT.

@zapov
Copy link
Member

zapov commented Jul 18, 2018

I've refactored some stuff so Java8 configuration should not be loaded by default anymore.
Also added convenience Settings.basicSetup() for initialization without Java8 stuff.
Let me know if you think something else is missing for older Android versions.

@varahash
Copy link
Contributor Author

I've run AndroidJava6 example with new version, and there are two issues:

  1. ImmutableAnalyzer should be refactored to support Java6, I got following error:
    java.lang.NoSuchMethodError: java.lang.reflect.Constructor.getParameterCount
        at com.dslplatform.json.runtime.ImmutableAnalyzer.analyze(ImmutableAnalyzer.java:137)
        at com.dslplatform.json.runtime.ImmutableAnalyzer.access$000(ImmutableAnalyzer.java:12)
        at com.dslplatform.json.runtime.ImmutableAnalyzer$1.tryCreate(ImmutableAnalyzer.java:89)
        at com.dslplatform.json.runtime.ImmutableAnalyzer$1.tryCreate(ImmutableAnalyzer.java:85)
        at com.dslplatform.json.DslJson.lookupWritersFromFactories(DslJson.java:1006)
        at com.dslplatform.json.DslJson.tryFindWriter(DslJson.java:970)
        at com.dslplatform.json.DslJson.tryFindWriter(DslJson.java:1156)
  1. DslJson unable to find writer (perhaps reader too) for classes which implements 'JsonObject'

@zapov
Copy link
Member

zapov commented Jul 20, 2018

The first one seems easily fixable (will fix it now)

Not sure what you mean by the second one. That tryFindWriter will return null? It was always like that. serialize anddeserialize should work though since they explicitly check for it.

@varahash
Copy link
Contributor Author

Your last fix is not fully compatible with Java6. Please look at how it done in my PR 3cacdd5#diff-837b9da4c6b09f28d602cf80cfa379de
The 'java.lang.reflect.Parameter' and 'java.util.Optional' must not be used.

Just run AndroidJava6 example project on Android simulator with api16 and you will see problem.

@zapov
Copy link
Member

zapov commented Jul 20, 2018

Ah, right. Can you finish it up on master? I won't have time until Monday

@varahash
Copy link
Contributor Author

Yes, I will provide PR

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

2 participants