Skip to content

OPENWIRE-69 - Fix PartialCommand and ProducerAck sequence ids#4

Merged
cshannon merged 2 commits intoapache:mainfrom
cshannon:openwire-69
Dec 7, 2023
Merged

OPENWIRE-69 - Fix PartialCommand and ProducerAck sequence ids#4
cshannon merged 2 commits intoapache:mainfrom
cshannon:openwire-69

Conversation

@cshannon
Copy link
Copy Markdown
Contributor

@cshannon cshannon commented Dec 6, 2023

PartialCommand and ProducerAck duplicate the same sequence id on more than one property. This commit fixes the sequence and also adds validation to the marshaller to verify that sequences are not negative or duplicated and that they are contiguous. If there is any issue with validation then an IllegalArgumentException is thrown.

I also added another check when creating the OpenWireTypeDescription object that the OpenWireType annotation exists.

PartialCommand and ProducerAck duplicate the same sequence id on more
than one property. This commit fixes the sequence and also adds
validation to the marshaller to verify that sequences are not duplicated
and if they are an IllegalArgumentException is thrown.
@cshannon cshannon requested a review from tabish121 December 6, 2023 21:39
@cshannon cshannon self-assigned this Dec 6, 2023
@cshannon
Copy link
Copy Markdown
Contributor Author

cshannon commented Dec 6, 2023

So I am going to mark this as a draft for now as it works but I think I will update this to be smarter. Right now it just looks for unique sequences which is a good start but I think we should be smarter and just iterate over it after we have sorted the list and also verify that there are no gaps. We wouldn't want sequence ids of something like 1,2,4 we would only want to allow 1,2,3 so I can fix that tomorrow and push a new commit.

@cshannon cshannon marked this pull request as draft December 6, 2023 21:46
@tabish121
Copy link
Copy Markdown
Contributor

The change makes sense to me, will review tomorrow if you push more updates. It's likely there's more areas where deeper validation would be useful to prevent user error when updating / adding to openwire commands.

@cshannon cshannon marked this pull request as ready for review December 7, 2023 13:03
@cshannon
Copy link
Copy Markdown
Contributor Author

cshannon commented Dec 7, 2023

@tabish121 - As i updated in my description, a lot more validatiion is now done. I am checking that sequences are positive, not contiguous and not duplicated. I also added a sanity check for the OpenWireType annotation to actually exist. As you noted a lot more validation like this probably needs ot be done elsewhere but this is a good start for this and more checks can be done in follow on issues.

If it looks ok I can squash and merge the PR.

@tabish121
Copy link
Copy Markdown
Contributor

Looks good, nice to validate all those cases for sure.

@cshannon cshannon merged commit db4b306 into apache:main Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants