Implemented file-containing-extension request for Reflection Service #1677
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
stefanadranca
changed the title
glbrntt
requested changes
Oct 19, 2023
| @@ -119,6 +149,15 @@ internal struct ReflectionServiceData: Sendable { | |||
| internal func nameOfFileContainingSymbol(named symbolName: String) -> String? { | |||
| return self.fileNameBySymbol[symbolName] | |||
| } | |||
|
|
|||
| internal func nameOfFileContainingExtension( | |||
| extendeeTypeName extendeeName: String, | |||
There was a problem hiding this comment.
Suggested change
| extendeeTypeName extendeeName: String, | |
| named extendeeName: String, |
Comment on lines
157
to
159
| return self.fileNameByExtensionDescriptor[ | ||
| ExtensionDescriptor(extendeeTypeName: extendeeName, fieldNumber: number) | ||
| ] |
There was a problem hiding this comment.
It's pretty weird to format it this way... better to make the key then lookup:
let key = ExtensionDescriptor(extendeeTypeName: extendeeName, fieldNumber: number)
return self.fileNameByExtensionDescriptor[key]
Comment on lines
133
to
137
| extension Sequence where Element == Google_Protobuf_FieldDescriptorProto { | ||
| var names: [String] { | ||
| self.map { $0.name } | ||
| } | ||
| } |
There was a problem hiding this comment.
This extension is so simple that I don't think it's worth defining, especially as it's only used in one place.
| extendeeTypeName: "InvalidType", | ||
| fieldNumber: 2 | ||
| ) | ||
| XCTAssertEqual(registryFileName, nil) |
There was a problem hiding this comment.
nit: use XCTAssertNil(registryFileName) (and elsewhere)
Comment on lines
209
to
231
| for fileDescriptorProto in receivedData { | ||
| if fileDescriptorProto == fileToFind { | ||
| XCTAssert( | ||
| fileDescriptorProto.extension.names.contains("extensionInputMessage1"), | ||
| """ | ||
| The response doesn't contain the serialized file descriptor proto \ | ||
| containing the \"extensionInputMessage1\" extension. | ||
| """ | ||
| ) | ||
| } else { | ||
| XCTAssert( | ||
| dependentProtos.contains(fileDescriptorProto), | ||
| """ | ||
| The \(fileDescriptorProto.name) is not a dependency of the \ | ||
| proto file containing the \"extensionInputMessage1\" symbol. | ||
| """ | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
This isn't a good test because receivedData could be an empty array which is clearly not right! If it's non-empty it's possible that it contains only fileToFind or only dependentProtos. In all cases the test would pass but shouldn't.
…ice. Motivation: The file-containing-extension request enables users to get the file descriptor protos of the proto file containing the extension they are looking for and its transitive dependencies. Modifications: - Created a new struct (ExtensionDescriptor) to represent the type that is extended and the field number of each extension that exists inside the protos passed to the Reflection Service. - Added a <ExtensionDescriptor, FileName> dictionary inside the ReflectionServiceData registry to store the extensions, avoid duplicates and retrieve nicely the file name of the proto that contains the requested extension. The file name is then used to get the serialized file descriptor protos of the proto containing the extension and its transitive dependencies. - Added integration and unit tests. Result: The Reflection Service can now be used to get the serialized file descriptor protos of the proto containing the provided extension and its transitive dependencies.
stefanadranca
force-pushed
the
sd-reflection-service
branch
from
8e8b34b
to
e7f6987
Compare
glbrntt
approved these changes
Oct 19, 2023
glbrntt
added
semver/minor
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.
Motivation:
The file-containing-extension request enables users to get the file descriptor protos of the proto file containing the extension they are looking for and its transitive dependencies.
Modifications:
Result:
The Reflection Service can now be used to get the serialized file descriptor protos of the proto containing the provided extension and its transitive dependencies.