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

PR to support Model enhancements #2056

Merged
merged 91 commits into from May 27, 2019
Merged

PR to support Model enhancements #2056

merged 91 commits into from May 27, 2019

Conversation

@dilipkrish
Copy link
Member

@dilipkrish dilipkrish commented Oct 1, 2017

#Model enhancements include

  • Support for JsonView

Part of a different effort (these are different concepts) edited 2017-10-07 10:51:59

  • Support for spring data rest projects
  • Support for inheritance
  • Possibly support all of these in the light of OAS 3.0
Copy link
Member Author

@dilipkrish dilipkrish left a comment

I made it a PR so we can add comments etc. I added some initial thoughts. I feel like the plugin still needs some tweaking. Let me know what you think?


public Optional<ResolvedType> extractProjection(ResolvedType type, List<Annotation> annotations, DocumentationType documentationType) {
ModelProjectionProviderPlugin selected =
modelProjectionProviders.getPluginFor(documentationType, new JacksonJsonViewProjectionProvider());

This comment has been minimized.

@dilipkrish

dilipkrish Oct 1, 2017
Author Member

I think if there are plugin extensibility's we should use all of them not just the first one. That way we can layer information via plugins.

}

@Override
public Optional<Class<?>> projectionFor(ResolvedType type, List<Annotation> typeAnnotations) {

This comment has been minimized.

@dilipkrish

dilipkrish Oct 1, 2017
Author Member

This should return a ResolvedType IMO.

Also have you thought about how we might influence a projection not using annotations (I don't know how gson handles this)? I think it might be a good idea to extract a parameter object i.e. projection context that gives it access to/or parts of the RequestMappingContext

This comment has been minimized.

@dilipkrish

dilipkrish Oct 1, 2017
Author Member

Oh never mind... I see what this is doing. I don't think this interface is accurate. I'm thinking of the return value as an alternate type for the type. This makes this interface is jackson specific and will not work for spring-data-rest I think.

I was thinking more in these terms. Lets take this example from here

public class Views {
    public static class Public {
    }
 
    public static class Internal extends Public {
    }
}

public class Item {
  
    @JsonView(Views.Public.class)
    public int id;
 
    @JsonView(Views.Public.class)
    public String itemName;
 
    @JsonView(Views.Internal.class)
    public String ownerName;
}

I would think that this interface would behave something like this.

//Pseudo code on how this should be used.

ProjectionContext context = new ProjectionContext(
          type, //Type to find a projection type
          annotations, //Annotations to help finding projections
          ... more params as needed
         );
JacksonJsonViewProjectionProvider plugin = createInstanceOfPlugin(); 
Optional<ResolvedType> projection = instance.projectionFor(context);
//OR
Optional<Model> projection = instance.projectionFor(context);

Here projection would be a either a in-memory ResolvedType of type ItemPublicView built using AlternateTypeBuilder or straight away building the Model object via the ModelBuilder, that has the public properties as we do here. What do you think?

This comment has been minimized.

@MaksimOrlov

MaksimOrlov Oct 1, 2017
Member

As i know, Gson does not have full projection support. Just only SerializationStrategy to determine which properties will be serialized.

This comment has been minimized.

@MaksimOrlov

MaksimOrlov Oct 1, 2017
Member

There is a method Optional <Class<?> getRequiredAnnotation() that will handle case with JsonView, and will be ignored in case of Spring Data Rest

This comment has been minimized.

@MaksimOrlov

MaksimOrlov Oct 1, 2017
Member

And we might have a case, where there won't be any projection, just all Class... For example, no defined @Projection classes, or no @JSONVIEW annotation on field.

This comment has been minimized.

@MaksimOrlov

MaksimOrlov Oct 1, 2017
Member

Dilip, that is outdated code....

 @Override
  public List<ResolvedType> projectionsFor(ResolvedType type, Optional<? extends Annotation> requiredAnnotation) {
    List<Class<?>> projections = newArrayList();
    if (requiredAnnotation.isPresent() &&
        requiredAnnotation.get() instanceof JsonView) {
      projections = newArrayList(((JsonView)requiredAnnotation.get()).value());
    }
    return from(projections).transform(toResolvedType()).toList();
  }

This comment has been minimized.

@MaksimOrlov

MaksimOrlov Oct 1, 2017
Member

In last commit I changed interface and implementation

This comment has been minimized.

@MaksimOrlov

MaksimOrlov Oct 1, 2017
Member

I agreed, ProjectionContext is a good idea, to consolidate all required information to determine projection

This comment has been minimized.

@MaksimOrlov

MaksimOrlov Oct 1, 2017
Member

But, for me Projection is ONLY a part of ModelContext, just additional information, to determine which properties must be serialized or deserialized.

@@ -187,6 +187,10 @@ public String getGroupName() {
public <T extends Annotation> Optional<T> findControllerAnnotation(Class<T> annotation) {
return handler.findControllerAnnotation(annotation);
}

public List<Annotation> getAnnotations() {

This comment has been minimized.

@dilipkrish

dilipkrish Oct 1, 2017
Author Member

Does the findControllerAnnotation findAnnotations not work? I feel like a plugin should request only annotations it knows about. This is very open interface in my opinion.

This comment has been minimized.

@MaksimOrlov

MaksimOrlov Oct 1, 2017
Member

I've deleted that already. The main idea is to make common interface to handle all cases with projections.

@@ -114,6 +116,11 @@ public String getName() {
}

@Override
public List<Annotation> getAnnotations() {

This comment has been minimized.

@dilipkrish

dilipkrish Oct 1, 2017
Author Member

Same here... I feel like a plugin should request only annotations it knows about. This is very open interface in my opinion.

This comment has been minimized.

@MaksimOrlov

MaksimOrlov Oct 1, 2017
Member

Agreed, last commit restored that.

@@ -0,0 +1 @@
/OperationResponseClassReader.java

This comment has been minimized.

@dilipkrish

dilipkrish Oct 1, 2017
Author Member

???

This comment has been minimized.

@MaksimOrlov

MaksimOrlov Oct 1, 2017
Member

Something wrong, I accidentally ignored.... I will fix it

This comment has been minimized.


import springfox.documentation.spi.DocumentationType;

public interface ProjectionProviderPlugin extends Plugin<DocumentationType> {

This comment has been minimized.

@dilipkrish

dilipkrish Oct 1, 2017
Author Member

I feel like this plugin shouldn have something other thanDocumentationType as the discriminator. It should be something like ProjectionType which can have one of the following types

  • Jackson
  • Spring-Data-Rest
  • Something we haven't thought off.

Then we can select the right plugin rather than pick the first one the pluginManager finds.Thoughts?

This comment has been minimized.

@MaksimOrlov

MaksimOrlov Oct 1, 2017
Member

For that we should have some explicit configuration. Any way we won't have all plugins at one time. So, JacksonJsonViewPlugin has the lowest priority. If Spring-Data-Rest is included in classpath it will overwrites with its own plugin with higher priority. And custom plugin has the highest priority will overwrite all plugins.

This comment has been minimized.

@MaksimOrlov

MaksimOrlov Oct 1, 2017
Member

ProjectionType - will be auto configurable. And Spring-Data-Rest will rewrite it with it's name. But I think it is better to overwrite all plugin, and use priority then to use the configuration plugin name.

* @param annotation - projections of type
* @return resolved projection names
*/
boolean applyProjection(ResolvedType activeProjection, ResolvedType typeToApply, List<ResolvedType> typeProjections);

This comment has been minimized.

@dilipkrish

dilipkrish Oct 1, 2017
Author Member

I did not understand this method. I'll bring up the code and take a look at it again.

This comment has been minimized.

@MaksimOrlov

MaksimOrlov Oct 1, 2017
Member

 @Override
  public boolean applyProjection(ResolvedType activeProjection, ResolvedType typeToApply,
      Optional<? extends Annotation> requiredAnnotation) {
    final Class<?> activeView = activeProjection.getErasedType();
    if (requiredAnnotation.isPresent() &&
        requiredAnnotation.get() instanceof JsonView &&
        activeView != null) {
      final Class<?>[] typeProjections = ((JsonView)requiredAnnotation.get()).value();
      int i = 0, len = typeProjections.length;
      for (; i < len; ++i) {
        if (typeProjections[i].isAssignableFrom(activeView)) {
          return true;
        }
      }
    }
    return false;
  }

This comment has been minimized.

@MaksimOrlov

MaksimOrlov Oct 1, 2017
Member

Determine if class field belongs to given projection or not

@springfox springfox deleted a comment Oct 1, 2017
@springfox springfox deleted a comment Oct 1, 2017
@springfox springfox deleted a comment Oct 1, 2017
@springfox springfox deleted a comment Oct 1, 2017
@springfox springfox deleted a comment Oct 1, 2017
@springfox springfox deleted a comment Oct 1, 2017
@springfox springfox deleted a comment Oct 1, 2017
@springfox springfox deleted a comment Oct 1, 2017
@springfox springfox deleted a comment Oct 13, 2017
@springfox springfox deleted a comment Oct 13, 2017
@springfox springfox deleted a comment Oct 13, 2017
@springfox springfox deleted a comment Oct 13, 2017
@springfox springfox deleted a comment Oct 13, 2017
@springfox springfox deleted a comment Oct 13, 2017
@springfox springfox deleted a comment Oct 13, 2017
@springfox springfox deleted a comment Oct 13, 2017
@PrabhuKandasamy
Copy link

@PrabhuKandasamy PrabhuKandasamy commented Jan 8, 2019

Hi, @PrabhuKandasamy.
I'm going to push a new algorithm that can handle cyclic references in a few weeks.

Ok. Waiting for that.

@vangogh-ken
Copy link

@vangogh-ken vangogh-ken commented Jan 10, 2019

wait for this fix

@phirzel
Copy link

@phirzel phirzel commented Feb 5, 2019

Hey, we would highly appreciate such an improvement as fast as possible.

(Currently we have to deploy each Service-Model resulting in a name clash on separate instances (for e.g. breaking changes, Openshift-Projects), which is not what we really want for operational reasons.)

@MaksimOrlov
Copy link
Member

@MaksimOrlov MaksimOrlov commented Feb 5, 2019

@phirzel
Almost done it. Going to clean little bugs on the next week and push the last changes.

@abdulhafidz
Copy link

@abdulhafidz abdulhafidz commented Feb 19, 2019

what happened

@MaksimOrlov
Copy link
Member

@MaksimOrlov MaksimOrlov commented Mar 24, 2019

I've pushed changes. Still a few questions remains opened.

@dilipkrish dilipkrish merged commit 40a71a6 into master May 27, 2019
2 of 4 checks passed
2 of 4 checks passed
codecov/patch 80.6% of diff hit (target 95.09%)
Details
codecov/project 93.39% (-1.7%) compared to 09d4a73
Details
License Compliance All checks passed.
Details
ci/circleci Your tests passed on CircleCI!
Details
@dilipkrish dilipkrish deleted the feature/model-enhancements branch May 27, 2019
@dilipkrish
Copy link
Member Author

@dilipkrish dilipkrish commented May 27, 2019

@MaksimOrlov thank you! thank you! thank you! for working on this! 🙇

For all of you waiting for this feature, please provide us feedback.

@dilipkrish dilipkrish removed the in progress label May 27, 2019
@dilipkrish dilipkrish added this to the 3.0 milestone May 27, 2019
@joao-rebelo
Copy link

@joao-rebelo joao-rebelo commented May 29, 2019

Hi,

Was having weird bugs since yesterday, untill I realized the snapshot version has changed! (the good and bad of working with snapshots)
For what I was seeing, it was duplicating my models for each usage in diferent methods.
So 3 methods returning ObjectA and I would get model ObjectA, ObjectA_1, ObjectA_2.

I wasn't able to easily make a simple project to exemplify this.. but if you need I can try.

Thanks for new developments either way!

Regards

@MaksimOrlov
Copy link
Member

@MaksimOrlov MaksimOrlov commented May 29, 2019

@joao-rebel, thanks for the reporting the issue.
Could you, please, provide code listing for the ObjectA and for the controllers(methods) that use that class?

@joao-rebelo
Copy link

@joao-rebelo joao-rebelo commented May 30, 2019

Hi,

Follows the snippet of my code:

RestController:

@RestController
@RequestMapping(path = "/basicdata/translation")
@Api(tags = "Translations", description = "Responsible for handling Translations", authorizations = {
		@Authorization("Bearer") })
public class TranslationRestController {

	@Autowired
	private TranslationService translationService;

	@ApiOperation(value = "Bulk get translations", notes = "Bulk returns a set of translations and returns a maps of Xentis IDs to a resolved multi-language translations")
	@GetMapping(path = "/bulk")
	@Nonnull
	public Mono<Map<HexId, MultiLangTranslation>> bulkGet(
			@ApiParam(value = "Ids of translations to fetch", required = true, allowMultiple = true) @RequestParam(name = "id", required = true) @Nonnull Set<HexId> theIds,
			@ApiParam(value = "Types of translations to get", required = true, allowMultiple = true) @RequestParam(name = "type", required = true) @Nonnull Set<TranslationType> theTypes) {
		return translationService.bulkGet(theIds, theTypes);
	}

	@ApiOperation(value = "Get all translations", notes = "Gets all the translations of the specified types and returns a maps of Xentis IDs to a resolved multi-language translations")
	@GetMapping(path = "")
	@Nonnull
	public Mono<Map<HexId, MultiLangTranslation>> getAll(
			@ApiParam(value = "Types of translations to get", required = true, allowMultiple = true) @RequestParam(name = "type", required = true) @Nonnull Set<TranslationType> theTypes) {
		return translationService.getAll(theTypes);
	}

	@ApiOperation(value = "Get all translations in a range", notes = "Gets all the translations within a specific range and returns a maps of Xentis IDs to a resolved multi-language translations")
	@GetMapping(path = "/range/{id}")
	@Nonnull
	public Mono<Map<HexId, MultiLangTranslation>> getRange(
			@ApiParam(value = "Sample Id within the provided range to fetch", required = true) @PathVariable(name = "id") @Nonnull HexId theId,
			@ApiParam(value = "Type of Xentis IDs Range to fetch translations for", required = true) @RequestParam(name = "range") @Nonnull RangeType theRange,
			@ApiParam(value = "Types of translations to get", required = true, allowMultiple = true) @RequestParam(name = "type", required = true) @Nonnull Set<TranslationType> theTypes) {
		return translationService.getRange(theId, theRange, theTypes);
	}
}

Docklet:

@Configuration
@EnableSwagger2WebFlux
public class SwaggerConfig {

	@Autowired
	private TypeResolver resolver;

	@Bean
	public Docket api() {
		return new Docket(DocumentationType.SWAGGER_2) //
				.ignoredParameterTypes(ServerWebExchange.class)
				.select()
				.apis(RequestHandlerSelectors.any())
				.paths(PathSelectors.any())
				.build()
				.apiInfo(apiEndPointsInfo())
				.groupName("Basic Data")
				.directModelSubstitute(HexId.class, HexIdModel.class)
				.alternateTypeRules(
						new RecursiveAlternateTypeRule(
								resolver,
								Arrays.asList(
										AlternateTypeRules.newRule(
												resolver.resolve(Mono.class, WildcardType.class),
												resolver.resolve(WildcardType.class)),
										AlternateTypeRules.newRule(
												resolver.resolve(ResponseEntity.class, WildcardType.class),
												resolver.resolve(WildcardType.class)))))
				.alternateTypeRules(
						new RecursiveAlternateTypeRule(
								resolver,
								Arrays.asList(
										AlternateTypeRules.newRule(
												resolver.resolve(Flux.class, WildcardType.class),
												resolver.resolve(List.class, WildcardType.class)),
										AlternateTypeRules.newRule(
												resolver.resolve(ResponseEntity.class, WildcardType.class),
												resolver.resolve(WildcardType.class)))));
	}

	private ApiInfo apiEndPointsInfo() {
		return new ApiInfoBuilder().title("Data API")
				.version("0.0.1")
				.build();

	}

	//For Swagger Editor compatibility.Check:https://github.com/swagger-api/swagger-core/issues/2944
	@Bean
	public JacksonModuleRegistrar swaggerJacksonModuleRegistrar() {
		return objectMapper -> ReferenceSerializationConfigurer.serializeAsComputedRef(objectMapper);
	}

the ObjectA, aka the MultiLangTranslation:

public class MultiLangTranslation extends HashMap<String, Translation> {

	/**
	 * The long <code>serialVersionUID</code>.
	 */
	private static final long serialVersionUID = 1360228790913298738L;
	//Marker class
}

The Translation:

public class Translation implements Serializable {

	/**
	 * The long <code>serialVersionUID</code>.
	 */
	private static final long serialVersionUID = -26163810284100442L;

	private String small;
	private String medium;
	private String big;
        .......

and the HexId:

public class HexId implements Serializable {

	private static final long serialVersionUID = 201711281450L;

	private long id;

	public HexId(String theHexValue) {
		this(Long.parseLong(theHexValue, 16));
	}

	public HexId(long theId) {
		id = theId;
	}

	public String getValue() {
		return Long.toHexString(id).toUpperCase();
	}

	public Long getId() {
		return id;
	}

Since my Service had 3 methods I was getting the Translation and the MultiLangTranslation repeated 3 times.
During my debug I was finding out that at a method (not finding it now in the old snapshot) which tries to find the root models from the Mono<Map<HexId, MultiLangTranslation>> it was only returning the HexId object, since the MultiLangTranslation extended from Map.

But! This also happened for other services where the principal objects didn't extended from Map, but instead referenced directly or indirectly this MultiLangTranslation.

Also for another service where the returns were only: Mono<Map<HexId, List>> is only generated one HexId model, but it didn't allow to make the directModelSubstitute(HexId.class, HexIdModel.class), as it would throw an exception saying that HexId would have been already registered.

Sory for the huge information, hope it can help..

@MaksimOrlov
Copy link
Member

@MaksimOrlov MaksimOrlov commented May 30, 2019

@joao-rebelo thanks a lot for this infromation, Could you please add the generated json schema file. And could you, please, provide full code listing for Translation class with getters and setter. As I can see class HexId is not symmetric (difference between getters and setters). So for serialization and deserialization will be generated two different models.

@joao-rebelo
Copy link

@joao-rebelo joao-rebelo commented May 30, 2019

Follows the translation. I believe it is symmetric.

public class Translation implements Serializable {

	/**
	 * The long <code>serialVersionUID</code>.
	 */
	private static final long serialVersionUID = -26163810284100442L;

	private String small;
	private String medium;
	private String big;

	public Translation() {
		//FOR SPRING DESERIALIZATION ONLY
	}

	public Translation(String theMedium) {
		small = null;
		medium = theMedium;
		big = null;
	}

	public Translation(String theSmall, String theMedium, String theBig) {
		small = theSmall;
		medium = theMedium;
		big = theBig;
	}

	public String getSmall() {
		return small;
	}

	public String getMedium() {
		return medium;
	}

	public String getBig() {
		return big;
	}

	@Override
	public String toString() {
		return new StringBuilder("{SMALL=").append(small).append(", MEDIUM=").append(medium).append(", BIG=").append(big).append("}").toString();
	}
}

The generated doc:

api-docs.txt

@MaksimOrlov
Copy link
Member

@MaksimOrlov MaksimOrlov commented May 30, 2019

Ok.
@joao-rebelo, I will make a test based on your listing and then fix everything, that looks like ambiguous at the swagger output file. Will back to you in a week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet