THRIFT-5544: add java code gen param to support including field annotation as metadata #2553

jimexist · 2022-04-01T02:00:35Z

add java code gen param to support including field annotation as metadata

Currently the code generator has a map of string to string field that denotes the "annotations" for a field:

thrift/compiler/cpp/src/thrift/parse/t_field.h

Line 109 in 90ea2e8

std::map<std::string, std::string> annotations_;

It is made available to generators such as JSON (in

thrift/compiler/cpp/src/thrift/generate/t_json_generator.cc

Lines 267 to 274 in 90ea2e8

    
           if (ttype->annotations_.size() > 0) { 
        
             write_key_and("annotations"); 
        
             start_object(); 
        
             for (auto & annotation : ttype->annotations_) { 
        
               write_key_and_string(annotation.first, annotation.second); 
        
             } 
        
             end_object(); 
        
           }

) to allow (runtime) interpretation of such metadata.

In Java, similar mechanism exists to register field metadata (via

thrift/compiler/cpp/src/thrift/generate/t_java_generator.cc

Lines 2862 to 2865 in 90ea2e8

    
           indent(out) << "org.apache.thrift.meta_data.FieldMetaData.addStructMetaDataMap(" 
        
                       << type_name(tstruct) << ".class, metaDataMap);" << endl; 
        
           indent_down(); 
        
           indent(out) << "}" << endl << endl;

which calls org.apache.thrift.meta_data.FieldMetaData#addStructMetaDataMap method to register into a centralized places. Such metadata is useful, e.g. the microservice framework Ameria uses it to dynamically proxy requests.

However the existing field metadata only contain information about field id, type, and name, but it does not contain annotations. Adding annotations will be useful for situations e.g. (these are useful enablement at user side, make possible if annotation is included)

some static metadata is useful at runtime for dynamic operations, e.g. like what deprecated means in the context of a field, adding obfuscated allows the user site to decide to obfuscate detailed information
allows for more richer and more type specific validation and transformation logic, e.g. since we can only have string as field type, adding date_format="YYYY-MM-DD" allows the thrift struct to be interpreted as a local date and thus can automatically be converted to java.time.LocalDate

This should be a back-compatible change as the default behavior is not to include the annotation. A second constructor can be added to allow included annotation to exist and register.

Did you create an Apache Jira ticket? (not required for trivial changes)
If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
Did you squash your changes to a single commit? (not required, but preferred)
Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
If your change does not involve any code, include [skip ci] anywhere in the commit message to free up build resources.

Jens-G · 2022-04-13T20:22:05Z

but a quick check shows it's defaults would change a lot of thrift code, and not always for the better.
I'm not sure if the Thrift community would endorse its use,

There were similar approaches a while ago and my (personal) feelings about automated C++ code formatting tools abilities are, well, let's call it "mixed feelings". But it's not only a C++ specific thing, I had the same experience with other tools and other languages as well. Sometimes the problems start with the style guide supplied by the manufacturer itself that ist a) the default and b) horribly broken. (Yes, I'm looking at you, Emba).

The best tool I've seen in that regard is surely gofmt. The style actually makes sense to begin with, and it also offers not a single formatting-related option that could be configured. None. Zero. Kind of "get some sane style without the bikeshedding" approach.

jimexist · 2022-04-14T01:26:27Z

but a quick check shows it's defaults would change a lot of thrift code, and not always for the better.
I'm not sure if the Thrift community would endorse its use,

There were similar approaches a while ago and my (personal) feelings about automated C++ code formatting tools abilities are, well, let's call it "mixed feelings". But it's not only a C++ specific thing, I had the same experience with other tools and other languages as well. Sometimes the problems start with the style guide supplied by the manufacturer itself that ist a) the default and b) horribly broken. (Yes, I'm looking at you, Emba).

The best tool I've seen in that regard is surely gofmt. The style actually makes sense to begin with, and it also offers not a single formatting-related option that could be configured. None. Zero. Kind of "get some sane style without the bikeshedding" approach.

@Jens-G did you mean to comment on #2559 (comment) instead?

jimexist · 2022-04-14T01:28:50Z

I'd say that both rustfmt and gofmt are in this category and it contribute to the fact that Rust's one of the most love languages (per stackoverflow)

Jens-G · 2022-04-14T07:09:58Z

did you mean to comment on [...]

Probably. Must have confused open tabs.

ctubbsii

I think this could use a larger explanation. From these changes, including the added help documentation in the generator options, it's not clear what 'annotation metadata' is. What are common examples of annotation metadata, and how would these be used? The JIRA doesn't have any details in it, either: https://issues.apache.org/jira/browse/THRIFT-5544

jimexist · 2022-04-18T07:02:43Z

I think this could use a larger explanation. From these changes, including the added help documentation in the generator options, it's not clear what 'annotation metadata' is. What are common examples of annotation metadata, and how would these be used? The JIRA doesn't have any details in it, either: https://issues.apache.org/jira/browse/THRIFT-5544

@ctubbsii thanks for the comment. detailed explanation added to jira ticket and inline

ctubbsii · 2022-04-18T16:32:05Z

I think this could use a larger explanation. From these changes, including the added help documentation in the generator options, it's not clear what 'annotation metadata' is. What are common examples of annotation metadata, and how would these be used? The JIRA doesn't have any details in it, either: https://issues.apache.org/jira/browse/THRIFT-5544

@ctubbsii thanks for the comment. detailed explanation added to jira ticket and inline

Thanks for the additional information. However, I'm still confused. Is this intended to serialize information about Java annotations @Override, @Deprecated, @Nullable, etc.? Are these annotations in the Java code that is generated from an *.thrift IDL file? Is the annotation defined in the *.thrift file or expected to be manually added after code generation?

I'm just not clear on the end-to-end use of this. Do you have a simple example?

jimexist · 2022-04-19T01:09:04Z

I think this could use a larger explanation. From these changes, including the added help documentation in the generator options, it's not clear what 'annotation metadata' is. What are common examples of annotation metadata, and how would these be used? The JIRA doesn't have any details in it, either: https://issues.apache.org/jira/browse/THRIFT-5544

@ctubbsii thanks for the comment. detailed explanation added to jira ticket and inline

Thanks for the additional information. However, I'm still confused. Is this intended to serialize information about Java annotations @Override, @Deprecated, @Nullable, etc.? Are these annotations in the Java code that is generated from an *.thrift IDL file? Is the annotation defined in the *.thrift file or expected to be manually added after code generation?

I'm just not clear on the end-to-end use of this. Do you have a simple example?

@ctubbsii i created a simple example at https://gist.github.com/Jimexist/7acffaa0c4967193937a05f56098853b with the newly added flag in this pull request.

ctubbsii · 2022-04-19T13:09:48Z

@ctubbsii i created a simple example at https://gist.github.com/Jimexist/7acffaa0c4967193937a05f56098853b with the newly added flag in this pull request.

Thanks. That example helps clear up some things. I was confused by the use of the terms "java" and "field annotation", because I was thinking Java's Field @Annotations.

I would call these "tags" rather than "annotations", in order to reduce confusion. However, if this is already an existing IDL language construct, then the name may already be established as "field annotations". If that's the case, I can propose some slight wording improvements in the gen-java parameter help doc.

ctubbsii

Here's some proposed naming / phrasing changes to clear up potential confusion.

compiler/cpp/src/thrift/generate/t_java_generator.cc

lib/java/gradle/generateTestThrift.gradle

jimexist · 2022-04-20T02:25:00Z

However, if this is already an existing IDL language construct, then the name may already be established as "field annotations".

@ctubbsii i created a simple example at https://gist.github.com/Jimexist/7acffaa0c4967193937a05f56098853b with the newly added flag in this pull request.

Thanks. That example helps clear up some things. I was confused by the use of the terms "java" and "field annotation", because I was thinking Java's Field @Annotations.

I would call these "tags" rather than "annotations", in order to reduce confusion. However, if this is already an existing IDL language construct, then the name may already be established as "field annotations". If that's the case, I can propose some slight wording improvements in the gen-java parameter help doc.

I agree it's confusing but i guess that ship is sailed: the name in C++ and also in all of the file names it's named as "annotation". I guess the best method is to try best to make it less ambiguous in Java.

ctubbsii

I have only this last final suggestion to help further clarify things in the documentation. But otherwise, this looks great to me.

compiler/cpp/src/thrift/generate/t_java_generator.cc

jimexist · 2022-04-21T01:30:29Z

had to do a rebase onto master due to conflict.

jimexist · 2022-04-21T07:19:42Z

seems like the build failure was due to kerl failing to compile erlang in time...

jimexist · 2022-04-30T10:13:56Z

now this is fixed, ready for merging

Convert TestAnnotationMetadata to JUnit5 API. This class was not included in THRIFT-5560 (PR #2574) when the conversion was done because this was a new class added in THRIFT-5544 (PR #2553), which was merged first.

jimexist changed the title ~~add java code gen param to support including field annotation as metadata~~ THRIFT-5544: add java code gen param to support including field annotation as metadata Apr 1, 2022

jimexist force-pushed the field-annotation-for-java branch 3 times, most recently from a927503 to a4089f9 Compare April 1, 2022 02:10

jimexist force-pushed the field-annotation-for-java branch 2 times, most recently from 337b061 to 4531c99 Compare April 17, 2022 01:44

ctubbsii reviewed Apr 17, 2022

View reviewed changes

jimexist force-pushed the field-annotation-for-java branch from 4531c99 to 33dae90 Compare April 19, 2022 00:59

ctubbsii reviewed Apr 19, 2022

View reviewed changes

This comment was marked as duplicate.

Sign in to view

ctubbsii reviewed Apr 20, 2022

View reviewed changes

compiler/cpp/src/thrift/generate/t_java_generator.cc Outdated Show resolved Hide resolved

ctubbsii approved these changes Apr 20, 2022

View reviewed changes

jimexist force-pushed the field-annotation-for-java branch 2 times, most recently from e7d1365 to b14e82a Compare April 21, 2022 01:30

jimexist force-pushed the field-annotation-for-java branch from b14e82a to 933287a Compare April 22, 2022 05:36

add field annotation metadata for java code gen

1384045

jimexist force-pushed the field-annotation-for-java branch from 933287a to 1384045 Compare April 27, 2022 11:47

Merge branch 'master' into field-annotation-for-java

dfd94d7

ctubbsii merged commit ada0865 into apache:master May 5, 2022

jimexist deleted the field-annotation-for-java branch May 6, 2022 00:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

THRIFT-5544: add java code gen param to support including field annotation as metadata #2553

THRIFT-5544: add java code gen param to support including field annotation as metadata #2553

jimexist commented Apr 1, 2022 •

edited

Loading

Jens-G commented Apr 13, 2022

jimexist commented Apr 14, 2022 •

edited

Loading

jimexist commented Apr 14, 2022 •

edited

Loading

Jens-G commented Apr 14, 2022

ctubbsii left a comment

jimexist commented Apr 18, 2022

ctubbsii commented Apr 18, 2022

jimexist commented Apr 19, 2022

ctubbsii commented Apr 19, 2022

ctubbsii left a comment

This comment was marked as duplicate.

jimexist commented Apr 20, 2022

ctubbsii left a comment

jimexist commented Apr 21, 2022

jimexist commented Apr 21, 2022

jimexist commented Apr 30, 2022

	if (ttype->annotations_.size() > 0) {
	write_key_and("annotations");
	start_object();
	for (auto & annotation : ttype->annotations_) {
	write_key_and_string(annotation.first, annotation.second);
	}
	end_object();
	}

	indent(out) << "org.apache.thrift.meta_data.FieldMetaData.addStructMetaDataMap("
	<< type_name(tstruct) << ".class, metaDataMap);" << endl;
	indent_down();
	indent(out) << "}" << endl << endl;

THRIFT-5544: add java code gen param to support including field annotation as metadata #2553

THRIFT-5544: add java code gen param to support including field annotation as metadata #2553

Conversation

jimexist commented Apr 1, 2022 • edited Loading

Jens-G commented Apr 13, 2022

jimexist commented Apr 14, 2022 • edited Loading

jimexist commented Apr 14, 2022 • edited Loading

Jens-G commented Apr 14, 2022

ctubbsii left a comment

Choose a reason for hiding this comment

jimexist commented Apr 18, 2022

ctubbsii commented Apr 18, 2022

jimexist commented Apr 19, 2022

ctubbsii commented Apr 19, 2022

ctubbsii left a comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

jimexist commented Apr 20, 2022

ctubbsii left a comment

Choose a reason for hiding this comment

jimexist commented Apr 21, 2022

jimexist commented Apr 21, 2022

jimexist commented Apr 30, 2022

jimexist commented Apr 1, 2022 •

edited

Loading

jimexist commented Apr 14, 2022 •

edited

Loading

jimexist commented Apr 14, 2022 •

edited

Loading