Support all_extension_numbers_of_type reflection requests #1680
Conversation
…ion Service. Motivation: The reflection service should provide the possibility for users to request the list with all the field numbers of the extensions of a type. Modifications: - Implemented the dictionary that stores the arrays of integers representing the field numbers of the extensions for each type that has extensions. - Implemented the methods of the Reflection Service that create the specific response with the array of integers representing the field numbers or an empty array for the case that the type doesn't have any extensions (but is a valid type). - Implemented integration and Unit tests. Result: The Reflection Service will enable users to find all the extension field numbers for a specific type they requested.
…ith a dot, and the typeName in the internal data structures of the registry is the fully qualified name of each type
| if self.fieldNumbersByType[typeName] == nil { | ||
| self.fieldNumbersByType[typeName] = [] | ||
| } | ||
| let numberPosition = self.fieldNumbersByType[typeName]!.count | ||
| self.fieldNumbersByType[typeName]?.insert( | ||
| `extension`.number, | ||
| at: numberPosition | ||
| ) |
There was a problem hiding this comment.
A couple of notes:
- Don't use
insertif you want to add an element to the end of the array, useappend - We don't need to check whether there's a value in the dictionary and then insert an empty array. Instead we can use https://developer.apple.com/documentation/swift/dictionary/subscript(_:default:)
| internal static func extractTypeNameFrom(fullyQualifiedName name: String) -> String { | ||
| var nameCopy = name | ||
| if nameCopy.first == "." { | ||
| nameCopy.removeFirst() | ||
| } | ||
| return nameCopy |
There was a problem hiding this comment.
We don't need a whole function for this, it's just a one liner:String(name.drop(while: { $0 == "." }))
| fieldNumber number: Int32 | ||
| ) -> String? { | ||
| let key = ExtensionDescriptor(extendeeTypeName: extendeeName, fieldNumber: number) | ||
| return self.fileNameByExtensionDescriptor[key] | ||
| } | ||
|
|
||
| internal func extensionsFieldNumbersOfType(named typeName: String) -> [Int32]? { |
There was a problem hiding this comment.
Optional collections are rarely the right thing to do. Are we sure that we should return [Int32]? here vs [Int32]?
There was a problem hiding this comment.
I was following the pattern used for the other requests too, meaning that the functions of the registry could return nil, when the thing we are requesting for doesn't exist and the Reflection Service itself "decides" whether or not it should throw an error / send an error response. Should I change this?
There was a problem hiding this comment.
Does it make sense to distinguish between a nil-array and an empty one? If it does make sense then don't change this but document the distinction.
| fieldNumber number: Int32 | ||
| ) -> String? { | ||
| let key = ExtensionDescriptor(extendeeTypeName: extendeeName, fieldNumber: number) | ||
| return self.fileNameByExtensionDescriptor[key] | ||
| } | ||
|
|
||
| // Returns nil if the type has no extensions or if it doesn't exist. |
There was a problem hiding this comment.
This behaviour is surprising: nil usually means the thing we were looking for didn't exist (e.g. consider dictionary["foo"]). Having it also return nil when the type exists but doesn't have any extensions isn't a good API because it places the onus on the caller to work out whether the type exists or not.
In this case we should probably just do the work upfront and ensure that fieldNumbersByType contains the right information, i.e. empty arrays when the type exists but has no extensions.
There was a problem hiding this comment.
This is still not right: the only reason we store messageTypeNames is to check whether a message exists in this function, it isn't used anywhere else.
When we create and populate fieldNumbersByType we should do so such it has a value for every type we know about, even if the value is an empty array. It means we do the work upfront and the semantics are clear: if a value exists then the type exists, if no value exists then we don't know about the type.
| fieldNumber number: Int32 | ||
| ) -> String? { | ||
| let key = ExtensionDescriptor(extendeeTypeName: extendeeName, fieldNumber: number) | ||
| return self.fileNameByExtensionDescriptor[key] | ||
| } | ||
|
|
||
| // Returns nil if the type has no extensions or if it doesn't exist. |
There was a problem hiding this comment.
This is still not right: the only reason we store messageTypeNames is to check whether a message exists in this function, it isn't used anywhere else.
When we create and populate fieldNumbersByType we should do so such it has a value for every type we know about, even if the value is an empty array. It means we do the work upfront and the semantics are clear: if a value exists then the type exists, if no value exists then we don't know about the type.
Motivation:
The reflection service should provide the possibility for users to request the list with all the field numbers of
the extensions of a type.
Modifications:
for each type that has extensions.
representing the field numbers or an empty array for the case that the type doesn't have any extensions (but is a valid type).
Result:
The Reflection Service will enable users to find all the extension field numbers for a specific type they requested.