Remove FieldInfo.childFieldInfo #4566
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Motivation: We have introduced `TypeSignature` not to parse a struct type in place when creating `DocService` but put the struct in the list of `StructInfo` and link to it using `TypeSignature` in line#422. We could have the definition of a struct in only one place so we could dedup the information. However, we have started to parse a bean(which is a struct) in place to represent a `@RequestObject` type in line#1557. To represent the type, `FieldInfo.childFieldInfo` was added and it turned out to be a bad decision. Because after that, we have lots of duplicate information on structs. Also, the way that some of the structs use a link using `TypeSignature` whereas the rest of them don't, brought us the inconsistency of parsing and reorganizing the `DocService`. Modifications: - Remove `FieldInfo.childFieldInfos`. - `FieldInfo` now has only a `TypeSignature` and uses the `TypeSignature` to link to the corresponding struct. - The nested struct is not parsed in place. - Add `REQUEST_OBJECT` `TypeSignature` for a `@RequestObject`. - `FieldInfo`'s name for a `@RequestObject` is now the parameter name. - It was `class.getSimpleName` before which doesn't represent the field information properly. Result: - Unified way to create and organize `DocService`.
minwoox
force-pushed
the
remove_childFieldInfos
branch
from
7823541 to
e87613c
Compare
ikhoon
reviewed
Dec 13, 2022
Comment on lines
221
to
226
| try { | ||
| System.err.println( | ||
| new ObjectMapper().writerWithDefaultPrettyPrinter().writeValueAsString(response)); | ||
| } catch (JsonProcessingException e) { | ||
| throw new RuntimeException(e); | ||
| } |
| @@ -489,6 +472,15 @@ private static DescriptiveTypeInfo newDescriptiveTypeInfo( | |||
| return descriptiveTypeInfo; | |||
| } | |||
|
|
|||
| if (typeSignature.type() == TypeSignatureType.REQUEST_OBJECT) { | |||
There was a problem hiding this comment.
RequestObjectTypeSignature could be exposed to users through a custom DescriptiveTypeInfoProvider. They might find it difficult to interpret TypeSignatureType.REQUEST_OBJECT. Because AnnotatedValueResolversWrapper is an internal class that users can't access and RequestObjectTypeSignature is an internal API.
I guess you don't want users to allow accessing RequestObjectTypeSignature since you located it in the internal package. If so, we should also hide REQUEST_OBJECT from the prospect of users' view? We may set the TypeSignatureType to TypeSignatureType.STRUCT and extract AnnotatedValueResolversWrapper if the type is RequestObjectTypeSignature.
Suggested change
| if (typeSignature.type() == TypeSignatureType.REQUEST_OBJECT) { | |
| if (typeSignature instanceof RequestObjectTypeSignature) { |
There was a problem hiding this comment.
This is a brilliant idea. 👍
jrhee17
approved these changes
Dec 14, 2022
...src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedDocServicePlugin.java
Show resolved
Hide resolved
Comment on lines
27
to
29
| RequestObjectTypeSignature(TypeSignatureType requestObject, String name, Class<?> type, | ||
| Object annotatedValueResolvers) { | ||
| super(requestObject, name, type); |
There was a problem hiding this comment.
Suggested change
| RequestObjectTypeSignature(TypeSignatureType requestObject, String name, Class<?> type, | |
| Object annotatedValueResolvers) { | |
| super(requestObject, name, type); | |
| RequestObjectTypeSignature(TypeSignatureType type, String name, Class<?> clazz, | |
| Object annotatedValueResolvers) { | |
| super(type, name, clazz); |
ikhoon
reviewed
Dec 14, 2022
Comment on lines
475
to
479
| if (typeSignature.type() == TypeSignatureType.REQUEST_OBJECT) { | ||
| if (typeSignature instanceof RequestObjectTypeSignature) { | ||
| final Object annotatedValueResolvers = | ||
| ((RequestObjectTypeSignature) typeSignature).annotatedValueResolvers(); | ||
| if (annotatedValueResolvers instanceof AnnotatedValueResolversWrapper) { |
There was a problem hiding this comment.
Should we move this block before DescriptiveTypeInfo descriptiveTypeInfo = provider.newDescriptiveTypeInfo(typeDescriptor);?
// RequestObjectTypeSignature which is internal class should not be exposed
// externally via the provider.
if (typeSignature instanceof RequestObjectTypeSignature) {
return ...
}
...
DescriptiveTypeInfo descriptiveTypeInfo = provider.newDescriptiveTypeInfo(typeDescriptor);There was a problem hiding this comment.
Ah yes, we should. 😉
ikhoon
approved these changes
Dec 14, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Remove
FieldInfo.childFieldInfoMotivation:
We have introduced
TypeSignaturenot to parse a struct type in place when creatingDocServicebut put the struct in the list ofStructInfoand link to it usingTypeSignaturein #422.We could have the definition of a struct in only one place so we could dedup the information.
However, we have started to parse a bean(which is a struct) in place to represent a
@RequestObjecttype in #1557.To represent the type,
FieldInfo.childFieldInfowas added and it turned out to be a bad decision.Because after that, we have lots of duplicate information on structs. Also, the way that some of the structs use a link using
TypeSignaturewhereas the rest of them don't, brought us the inconsistency of parsing and reorganizing theDocService.Modifications:
FieldInfo.childFieldInfos.FieldInfonow has only aTypeSignatureand uses theTypeSignatureto link to the corresponding struct.REQUEST_OBJECTTypeSignaturefor a@RequestObject.FieldInfo's name for a@RequestObjectis now the parameter name.class.getSimpleNamebefore which doesn't represent the field information properly.Result:
DocService.