handle duplicate types #45
Conversation
| // Filter out duplicate types. | ||
| // This will avoid that searching a type by name will return more than one result. | ||
| const seenTypes = new Set<string>(); | ||
| const uniqueTypes = types.filter((definition) => { |
There was a problem hiding this comment.
why are we deduping instead of making the search function able to return multiple or return the first that matches?
Should having duplicate types be a schema error?
There was a problem hiding this comment.
+1 - seems weird to me that we would have duplicate types. At least it seems like we should confirm they are identical and throw an error if not (or always throw an error)
There was a problem hiding this comment.
Turns out that duplicates were erroneously produced by mcap convert, so we have to handle that case. I changed it now to a deep comparison instead of only checking the name.
There was a problem hiding this comment.
Is this a bug in ROS?
How do we know which definition of the type name is the correct one if it appears twice in the combined definitions format? Like what if the same name had different definitions should we be throwing an error if they don't match?
We've never seen this duplication in a combined definition string have we? I'm trying to understand if this is bad data and we've correctly errored loudly or how this should be handled. My current feedback is that I view the input as malformed.
Separate from that - is it that parsing is failing on this input or something upstream in studio that made this assumption? Maybe it is correct that the type appears twice from the parse output and it is upstream's responsibility to understand that is allowed?
| // This will avoid that searching a type by name will return more than one result. | ||
| const seenTypes: MessageDefinition[] = []; | ||
| const uniqueTypes = types.filter((definition) => { | ||
| return seenTypes.find((otherDefinition) => isMsgDefEqual(definition, otherDefinition)) |
There was a problem hiding this comment.
is this deduplication as performant as it could be or as performant as _.uniqBy? I guess it's only run once per schema on startup so it's not that big of a deal.
There was a problem hiding this comment.
is this deduplication as performant as it could be or as performant as
_.uniqBy?
Probably not. OK to add lodash as a dependency? If so then I'll change it to use `_.uniqBy
There was a problem hiding this comment.
Can you please not add a dependency to this project for a bug fix?
There was a problem hiding this comment.
Yeah I'd avoid adding the dep. Just wanted to see if we could get this down to a single iteration through the types, but it's not strictly necessary
|
Actually it doesn't look like to me that the comments were addressed. Why shouldn't we error on these schemas instead of deduping them? |
It turned out that |
src/parse.ts
Outdated
| // Filter out duplicate types. | ||
| // This will avoid that searching a type by name will return more than one result. |
There was a problem hiding this comment.
Would be helpful to add some historical context about mcap convert, to explain why we shouldn't delete this code
Public-Facing Changes
Fix exception when message definition contains duplicate types
Description
This PR removes duplicate types (with the same name) to ensure that at max one result is returned when searching a type by its name. This fixes an exception that occurred when the message definition contained duplicate types due to a bug in
mcap convert.