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.

Sign up for GitHub

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

feat: add field annotations merging functionality #83

Merged
merged 2 commits into from
Apr 13, 2022
Merged

feat: add field annotations merging functionality #83

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
22cfe45
feat: add field properties merging functionality
vam-google Apr 13, 2022
18d17d5
update test data to match better actual case from GCE
vam-google Apr 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 34 additions & 14 deletions src/main/java/com/google/cloud/discotoproto3converter/proto3/ProtoMerger.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,14 @@
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.stream.Collectors;

// This class does not intend to do a fully-functional merge of two proto models, instead it focuses
// on merging currently known potential discrepancies between old and new proto files:
// - Missing field in a new proto file
// - Missing values in enums
// - Mismatching service method google.api.method_signature option values
// - missing field in a new proto file;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't these all "old overwrites new"? Maybe we should just have a mention right before the list: "carrying over elements from the old version of the proto into the new version when doing a pure conversion would result in those elements being missing or mismatched"

// - missing values in enums;
// - mismatching service method google.api.method_signature option values (old overwrite new);
// - mismatching field annotations (old overwrite new).
//
// Additional merging functionality is expected to be added to this class as needed.
public class ProtoMerger {
Expand Down Expand Up @@ -79,32 +81,50 @@ private void mergeMessages(Map<String, Message> newMessages, Map<String, Message
mergeFields(newMessage, oldMessage, newMessages);

// Merge enums
Map<String, Message> newEnumsMap = new HashMap<>();
for (Message nestedEnum : newMessage.getEnums()) {
newEnumsMap.put(nestedEnum.getName(), nestedEnum);
}
for (Message oldEnum : oldMessage.getEnums()) {
Message newEnum = newEnumsMap.get(oldEnum.getName());
mergeFields(newEnum, oldEnum, newMessages);
}
mergeEnums(newMessages, oldMessage, newMessage);
}
}

private void mergeEnums(
Map<String, Message> newMessages, Message oldMessage, Message newMessage) {
Map<String, Message> newEnumsMap = new HashMap<>();
for (Message nestedEnum : newMessage.getEnums()) {
newEnumsMap.put(nestedEnum.getName(), nestedEnum);
}
for (Message oldEnum : oldMessage.getEnums()) {
Message newEnum = newEnumsMap.get(oldEnum.getName());
mergeFields(newEnum, oldEnum, newMessages);
}
}

private void mergeFields(
Message newMessage, Message oldMessage, Map<String, Message> newMessages) {
// Merge fields
Map<String, Field> newFieldsMap =
newMessage.getFields().stream().collect(Collectors.toMap(ProtoElement::getName, f -> f));

for (Field oldField : oldMessage.getFields()) {
// compareTo() will be used by contains() to search for the field, not equals().
if (!newMessage.getFields().contains(oldField)) {
newMessage.getFields().add(copyField(oldField, newMessages));
// Copy removed field
newMessage.getFields().add(copyField(null, oldField, newMessages));
} else {
Field newField = newFieldsMap.get(oldField.getName());
// Copy missing options if a new field has less options than old field.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitty nits: s/less/fewer/; s/new field/newField/ s/old field/oldField/

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very nitty nit =)

// This is a very primitive merge logic. Add a proper merge logic if ever needed.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider making this comment a TODO.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The merger is basically written (and it matches the comment at the top of the merger class), that it merges only what is required and there are many potentially missing functions, which are expected to be added if needed. This specific case is not special compared to others, that is why I guess having todo for this one but not having for others would be weird.

if (oldField.getOptions().size() > newField.getOptions().size()
|| oldField.isOptional() != newField.isOptional()) {
newMessage.getFields().remove(oldField);
newMessage.getFields().add(copyField(newField, oldField, newMessages));
}
}
}
}

// We need to replace references for message types from oldMessages to newMessages.
// Despite message types having the same names, they are two independently created
// sets of objects.
private Field copyField(Field oldField, Map<String, Message> newMessages) {
private Field copyField(Field newField, Field oldField, Map<String, Message> newMessages) {
Message valueType = Message.PRIMITIVES.get(oldField.getValueType().getName());
if (valueType == null) {
valueType = newMessages.get(oldField.getValueType().getName());
Expand All @@ -123,7 +143,7 @@ private Field copyField(Field oldField, Map<String, Message> newMessages) {
oldField.isRepeated(),
oldField.isOptional(),
keyType,
oldField.getDescription(),
newField != null ? newField.getDescription() : oldField.getDescription(),
oldField.isFirstInOrder());

// Do not do deep copy for options because they are all generic immutable objects (strings or
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/google/cloud/discotoproto3converter/proto3/ProtoParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ private Field parseField(String fieldString, SortedMap<String, Message> nestedEn
}
Message keyType = Message.PRIMITIVES.get(m.group("keyType"));
boolean repeated = m.group("repeated") != null || keyType != null;
boolean optional = m.group("optional") != null;
boolean optional = repeated || m.group("optional") != null;

Field parsedField =
new Field(m.group("name"), valueType, repeated, optional, keyType, null, false);
Expand Down
2 changes: 1 addition & 1 deletion src/test/resources/google/cloud/compute/v1small/compute.merge.proto.baseline
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,7 @@ message DeleteAddressRequest {
// For example, consider a situation where you make an initial request and the request times out. If you make the request again with the same request ID, the server can check if original operation with the same request ID was received, and if so, will ignore the second request. This prevents clients from accidentally creating duplicate commitments.
//
// The request ID must be a valid UUID with the exception that zero UUID is not supported (00000000-0000-0000-0000-000000000000).
optional string request_id = 37109963;
optional string request_id = 37109963 [(google.api.field_behavior) = REQUIRED];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made the mistake of not removing the proto3_optional label in my original PR, and had to fix it here googleapis/googleapis#712. Can we exclude such a label if the field_behavior is REQUIRED?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(totally meant to send this comment this morning but didn't press the button)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic just preserves the old situation - if it was optional, then it will remain optional, if it idid not have optional, it will remain non-optional. This specific thing is just a test. I modified the test data to represent better the actual case in GCE.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks!


}

Expand Down
3 changes: 1 addition & 2 deletions src/test/resources/google/cloud/compute/v1small/compute.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,8 +327,7 @@ message DeleteAddressRequest {
// For example, consider a situation where you make an initial request and the request times out. If you make the request again with the same request ID, the server can check if original operation with the same request ID was received, and if so, will ignore the second request. This prevents clients from accidentally creating duplicate commitments.
//
// The request ID must be a valid UUID with the exception that zero UUID is not supported (00000000-0000-0000-0000-000000000000).
optional string request_id = 37109963;

optional string request_id = 37109963 [(google.api.field_behavior) = REQUIRED];
}

// [Output Only] If errors are generated during processing of the operation, this field will be populated.
Expand Down