Support Thrift TypeDef in DocService

[minwoox]

Motivation:
Currently, we don't resolve the Thrift typedef fields correctly.

Modifications:

• Resolve Thrift typedef fields correctly using Field.getGenericType().
• Use i8 instead of byte for Thrift 0.12+ to remove the warning.

Result:

• The Thrift typedefs are correctly resolved.

[ikhoon]

Nice! 🙇‍♂️

[jrhee17]

Looks good! Left a minor question 🙏

[jrhee17]

Question) Is it possible to just use java reflection and clean up the old logic?
I ask because if a typedef is somewhere nested inside the struct, we will use generics instead of thrift objects to traverse the object tree anyways

Suggested change

	if (!fieldValueMetaData.isTypedef() && !hasTypeDef(fieldValueMetaData)) {
	return toTypeSignature(fieldValueMetaData);
	}
	try {
	final Field field = parentType.getField(fieldMetaData.fieldName);
	return toTypeSignature(field.getGenericType());
	} catch (NoSuchFieldException e) {
	// Ignore exception.
	}
	return TypeSignature.ofUnresolved(firstNonNull(fieldValueMetaData.getTypedefName(), "unknown"));
	try {
	final Field field = parentType.getField(fieldMetaData.fieldName);
	return toTypeSignature(field.getGenericType());
	} catch (NoSuchFieldException e) {
	// Ignore exception.
	}
	return TypeSignature.ofUnresolved(firstNonNull(fieldValueMetaData.getTypedefName(), "unknown"));

[minwoox]

if a typedef is somewhere nested inside the struct, we will use generics instead of thrift objects to traverse the object tree anyways

Yes, exactly. That's why I added hasTypeDef method.

I left the old logic because there are not so many people who use type def with Armeria(type def doesn't do anything with Java).
In that case, I thought using explicit metadata instead of using implicit reflection is better.
But not so strong on this because we can integrate the logic into one if we just use reflection.
So please let me know if you think it's better to use just reflection for all cases.

[jrhee17]

My original concern was that if we have to troubleshoot anything, we would have to guess which code path was used.
Having said this, I'm also fine to go ahead with the fix and clean up later if necessary.

[jrhee17]

type def doesn't do anything with Java

I think I've seen some usage with typedef to abstract logic within the thrift idl (especially if the idl becomes very long and complex)

[minwoox]

I got your concern. If so, what if I extract them as a method and add documentation for that? It will make it clear?

[minwoox]

I think I've seen some usage with typedef to abstract logic within the thrift idl

Oops, I didn't know about it. 😓
So are they using DocService with the type definition?

[ikhoon]

Sorry for merging the PR in the middle of the discussion.
Personally, I prefer to use expose the original type because a type alias may be used to share a type in multiple structures but DocService shows one structure at a time.

We may improve DocService to display both type information and alias properly somehow.

[minwoox]

We may improve DocService to display both type information and alias properly somehow.

Indeed. 😊

[jrhee17]

Question) Is there a particular exception that we wanted to ignore? If not, I wonder if debugging would help with troubleshooting potential issues

[minwoox]

I thought NoSuchFieldException won't be raised and just added the catch clause because it's a checked exception.
So you mean we need to at least leave a debug log?

[jrhee17]

Oh I see, I guess there's no need to leave a log if we're sure this won't happen 😅

[jrhee17]

Thanks for the fix @minwoox ! 🙇 👍 🙇