katharsis-project / katharsis-framework

Katharsis adds powerful layer for RESTful endpoints providing implementenation of JSON:API standard
http://katharsis.io
Apache License 2.0
135 stars 65 forks source link

Katharsis 2.8.2 upgrade issue #307

Open zlhades3 opened 7 years ago

zlhades3 commented 7 years ago

I upgrade Katharsis from 2.7.0 to 2.8.2 And I found below issue.

Katharsis cannot discovery repositories which there is no empty-argument constructor. Temp solution: Add empty-argument constructor for each repositories Katharsis cannot discovery exception mapper. Temp solution: add below code in KatharsisFeature constructor.

this.addModule(new CoreModule("io.katharsis.example.jersey.domain", this.buildResourceFieldNameTransformer(objectMapper) )); I fixed those 2 issues by debug the source. but maybe is bad practices.

What's the best practices in 2.8.2 to configure KatharsisFeature?

Pls reference to Katharsis's example: https://github.com/katharsis-project/katharsis-framework/tree/master/katharsis-examples/jersey-example

Feature class https://github.com/katharsis-project/katharsis-framework/blob/master/katharsis-examples/jersey-example/src/main/java/io/katharsis/example/jersey/KatharsisDynamicFeature.java

http://stackoverflow.com/questions/41764567/katharsis-2-8-2-upgrade-issue/41795597#41795597

remmeier commented 7 years ago

there are a lot of different ways katharsis can be integrated. Are you using something like Spring or CDI? Then you can let the dependency injection framework takeover the work of creating and resolving the various objects.

And are you still using QueryParams or QuerySpec?

You may have a look at the Spring or Wildlfy example. Ideally you make use of the DefaultConstructor of KatharsisFeature.

zlhades3 commented 7 years ago

We use google juice and jersey currently.

Currently we try replace QueryParams with QuerySpec, and we found many issue. I will try to address it in other comments.

This what I have done: @Inject public KatharsisDynamicFeature(ObjectMapper objectMapper, Injector injector) { super(objectMapper, new DefaultQuerySpecDeserializer(), injector::getInstance); this.injector = injector; this.addModule(new CoreModule(KatharsisApplication.API_PACKAGE_NAME, new ResourceFieldNameTransformer(objectMapper.getSerializationConfig()))); }

This code work, but not stable, I may not init KatharsisFeature, Could you give me some suggestion? Thanks.

zlhades3 commented 7 years ago

Other issues found when we replace QueryParams with QuerySpec:

  1. Cannot pass prams which not exist in resource. e.g.: Task resource only 2 attributes(id, name) exposed, but I want filter by "time" which not exist in Task resource. Temp solution, add "time" attribute in Task with @JsonIgnore.

  2. No way to add custom parameter parser for querySpec. And StandardTypeParser cannot implement in other package. e.g.: filter[date][eq]=2015-01-01 will get exception after switch to querySpec.

  3. "-" is not allowed in resource. e.g.: "filter[Main-Task]","filter[Sub-Task]" will be reject.

  4. Inconsistences solution in sort e.g.: Input : page[limit]=2&sort=-id Page Link: page[limit]=2&sort[Main-Task]=-id

Seems we have to re-write DefaultQuerySpecDeserializer. Could you also provide us some suggestion?

masterspambot commented 7 years ago

@zlhades3 Can you create separate issue for every problem you have discover? That would ease us step by step solving them atomically.

zlhades3 commented 7 years ago

@masterspambot OK, thx.

remmeier commented 7 years ago

you mean guice I guess? In that case you have to provide a ServiceDiscovery implementation for guice just like katharsis-cdi or katharsis-spring. That would be very welcomed as PR. You can then pass the ServiceDiscovery to KatharsisFeature and it will directly use guice to resolve everything.

remmeier commented 7 years ago

you can make use of KatharsisFeature() default constructor.

zlhades3 commented 7 years ago

@remmeier you mean guice I guess? Yes

I saw ServiceDiscovery scan all resource at init stage. Guice only cannot get instance before resist in Guice. For Guice, may still keep use ReflectionsServiceDiscovery.

After do some investigation found the reason why exception mapping lost during init.

In ReflectionsServiceDiscovery getInstancesByType method. When we pass JsonApiExceptionMapper and try to get all exception mapper, the method return empty list.

Root cause: Our exception mapper implmented from io.katharsis.errorhandling.mapper.ExceptionMapper rather than JsonApiExceptionMapper, and io.katharsis is not in scan package, so that our application cannot load those exception mapper. MyMapper extend ExceptionMapper extened JsonApiExceptionMapper. If ExceptionMapper not existing sacn page, reflections cannot load MyMapper by type of JsonApiExceptionMapper

Solution: Add io.katharsis in KatharsisProperties.RESOURCE_SEARCH_PACKAGE property

zlhades3 commented 7 years ago

@remmeier BTW, Other people may have the same issue. Do you plan add JsonApiExceptionMapper in ReflectionsServiceDiscovery

e.g: filter.includePackage(JsonApiExceptionMapper.class.getPackage().getName()); builder = builder.addUrls(ClasspathHelper.forClass(JsonApiExceptionMapper.class));