Add "Override local binding" to HTTP Sender#333
Conversation
|
No AI was used in the making of this PR, only good ol' manual copy-paste. |
|
This PR probably needs some unit tests. I'll work on that at the end of this week. |
fba0393 to
4221c0a
Compare
There was a problem hiding this comment.
Pull request overview
Adds an “Override Local Binding” option to the HTTP Sender so outbound HTTP connections (and the “Test Connection” action) can bind to a specific local source IP, aligning HTTP Sender capabilities with other connectors.
Changes:
- Added
overrideLocalBinding+localAddressproperties to HTTP dispatcher settings and wired them through the client UI. - Applied Apache HTTP Client
RequestConfig.Builder#setLocalAddress(...)when override is enabled. - Extended connection testing to optionally bind a local address during socket connection tests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| server/src/com/mirth/connect/server/util/ConnectorUtil.java | Adds an overload to allow connection tests to bind to a local address. |
| server/src/com/mirth/connect/connectors/http/HttpDispatcherProperties.java | Introduces new HTTP sender properties for overriding local binding and storing a local address. |
| server/src/com/mirth/connect/connectors/http/HttpDispatcher.java | Configures Apache HTTP client requests to bind to the configured local address when enabled. |
| server/src/com/mirth/connect/connectors/http/HttpConnectorServlet.java | Updates “Test Connection” to optionally bind the local address when override is enabled. |
| client/src/com/mirth/connect/connectors/http/HttpSender.java | Adds UI controls for “Override Local Binding” and “Local Address”, plus validation/reset behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Joachim Beckers <joachim@jbeckers.be>
mgaffigan
left a comment
There was a problem hiding this comment.
Looks like a nice feature! I think some additional work is required to correct the velocity issue, and to fix the DCO check, but after that it looks good to me.
Thanks for sharing!
Test Results 111 files 214 suites 5m 45s ⏱️ Results for commit 82c7915. |
mgaffigan
left a comment
There was a problem hiding this comment.
@jbeckers - looks all good. Can you fix the DCO? Need that before we can approve.
kpalang
left a comment
There was a problem hiding this comment.
Some changes/clarifications required.
Review comments formatted as conventional comments
| /* | ||
| * Copyright (c) Mirth Corporation. All rights reserved. | ||
| * | ||
| * | ||
| * http://www.mirthcorp.com | ||
| * | ||
| * | ||
| * The software in this package is published under the terms of the MPL license a copy of which has | ||
| * been included with this distribution in the LICENSE.txt file. | ||
| */ |
There was a problem hiding this comment.
nitpick: While you're here I'd love to see the copyright notice be re-formatted in SPDX format. An example can be found here.
| } | ||
|
|
||
| if (props.isOverrideLocalBinding()) { | ||
| if (props.getLocalAddress().length() <= 3) { |
There was a problem hiding this comment.
question: Why is 3 significant?
| private void overrideLocalBindingYesRadioActionPerformed(ActionEvent evt) { | ||
| localAddressField.setEnabled(true); | ||
| localAddressLabel.setEnabled(true); | ||
| } | ||
|
|
||
| private void overrideLocalBindingNoRadioActionPerformed(ActionEvent evt) { | ||
| localAddressField.setEnabled(false); | ||
| localAddressLabel.setEnabled(false); | ||
| } |
There was a problem hiding this comment.
issue: No need to carry the ActionEvent as an argument if it is not used.
| /* | ||
| * Copyright (c) Mirth Corporation. All rights reserved. | ||
| * | ||
| * | ||
| * http://www.mirthcorp.com | ||
| * | ||
| * | ||
| * The software in this package is published under the terms of the MPL license a copy of which has | ||
| * been included with this distribution in the LICENSE.txt file. | ||
| */ |
There was a problem hiding this comment.
nitpick: While you're here I'd love to see the copyright notice be re-formatted in SPDX format. An example can be found here.
| final int port; | ||
| if (url.getPort() != -1) { | ||
| port = url.getPort(); | ||
| } else if (Strings.CI.equals(url.getProtocol(), "https")) { |
There was a problem hiding this comment.
thought: Tbh I'm not a fan of a library call for such simple comparison. I might be leaning towards "https".equalsIgnoreCase(url.getProtocol()).
| final RequestConfig.Builder requestConfigBuilder = RequestConfig.custom() | ||
| .setConnectTimeout(socketTimeout) | ||
| .setSocketTimeout(socketTimeout) | ||
| .setStaleConnectionCheckEnabled(true); |
There was a problem hiding this comment.
praise: Love the improved readability by splitting each builder function to a new line.
| /* | ||
| * Copyright (c) Mirth Corporation. All rights reserved. | ||
| * | ||
| * | ||
| * http://www.mirthcorp.com | ||
| * | ||
| * | ||
| * The software in this package is published under the terms of the MPL license a copy of which has | ||
| * been included with this distribution in the LICENSE.txt file. | ||
| */ |
There was a problem hiding this comment.
nitpick: While you're here I'd love to see the copyright notice be re-formatted in SPDX format. An example can be found here.





Fixes #332