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.

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

Fix: enforce strict TCPEndpoint build #235

Merged
merged 6 commits into from
Apr 25, 2024
Merged

Fix: enforce strict TCPEndpoint build #235

merged 6 commits into from
Apr 25, 2024

Conversation

678098
Copy link
Collaborator

@678098 678098 commented Apr 2, 2024

We have 2 similar routines:

bool TCPEndpoint::fromUri(const bsl::string& uri)

void TCPEndpoint::fromUriRaw(const bsl::string& uri)

The difference is that fromUriRaw does not return a bool result, does not perform as much checks and instead relies on a text usage contract Expects uri to be valid format.

In my opinion, the current implementation is error-prone, and we can enforce usage contract on a code level instead, at the price of some additional runtime checks.

Alternatively, I would prefer to remove fromUriRaw routine since we use it only within mwc, replacing its in-place usage with fromUri.

@678098 678098 requested a review from a team as a code owner April 2, 2024 10:30
@678098
Copy link
Collaborator Author

678098 commented Apr 2, 2024

Also note that we don't have unit tests for this component

@pniedzielski
Copy link
Collaborator

Alternatively, I would prefer to remove fromUriRaw routine since we use it only within mwc, replacing its in-place usage with fromUri.

In fact, I only see one usage of fromUriRaw, which is in the assign member function of the same class. This does some string concatenation of its parameters (which have no contract) and then passes the result to fromUriRaw without any validation. This is a contract error, and so we'd either need to change contract of assign or use fromUri to perform validation. I'm also in support of the latter.

However, I don't know whether we can remove fromUriRaw entirely, as there may be external package users of this function. If we can show that there are no packages using this function, it may be safe to remove. Maybe best course of action is first the changes in this patch, then next make assign use fromUri, then finally mark fromUriRaw as deprecated?

@678098 678098 merged commit fed1537 into main Apr 25, 2024
15 checks passed
@678098 678098 deleted the 678098-patch-2 branch April 25, 2024 18:27
alexander-e1off pushed a commit to alexander-e1off/blazingmq that referenced this pull request Oct 24, 2024
* Fix: enforce strict TCPEndpoint build

Signed-off-by: Evgeny Malygin <[email protected]>
alexander-e1off pushed a commit to alexander-e1off/blazingmq that referenced this pull request Oct 24, 2024
* Fix: enforce strict TCPEndpoint build

Signed-off-by: Evgeny Malygin <[email protected]>
alexander-e1off pushed a commit to alexander-e1off/blazingmq that referenced this pull request Oct 24, 2024
* Fix: enforce strict TCPEndpoint build

Signed-off-by: Evgeny Malygin <[email protected]>
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