MANGO-1817 - fix build methods in IpNetworkBuilder to get the Local IP Address, Broadcast Address and Subnet Mask #109

kishorevenki · 2025-01-08T16:51:29Z

MANGO-1817 - fix build methods to get IP Address, Broadcast Address and SubnetMask that are assigned to a NIC
MANGO-1818 - Deprecate withReuseAddress in IpNetworkBuilder as this flag would be set based on the OS
MANGO-1832 - Add new method to build the network from the interface name in IpNetworkBuilder
Add Unit Tests for the above feature IpNetworkBuilderTest
Remove usage withReuseAddress in ahoc/IPMasterTest and adhoc/MultipleLocalDevices

…P address, Broadcast and SubnetMask assigned to the NIC. MANGO-1818 - Deprecate withReuseAddress in IpNetworkBuilder as this flag would be set based on the OS MANGO-1832 - Add new method to build the network from the interface name in IpNetworkBuilder Add Unit Tests for the above feature IpNetworkBuilderTest Remove usage withReuseAddress in ahoc/IPMasterTest and adhoc/MultipleLocalDevices Change BACnet4J revision from 6.0.1-SNAPSHOT to 6.1.0-SNAPSHOT

src/main/java/com/serotonin/bacnet4j/npdu/ip/IpNetworkBuilder.java

src/test/java/com/serotonin/bacnet4j/adhoc/IpMasterTest.java

terrypacker · 2025-01-10T00:43:07Z

Generally formatting and approach looks good, I can't comment on the accuracy of the logic as I would need to dig deeper into this. I'm pushing for integration into our TeamCity CI server so we can get the test checks done on this PR instead of having each of us check it out and run them locally.

src/main/java/com/serotonin/bacnet4j/npdu/ip/IpNetworkBuilder.java

jazdw · 2025-01-10T17:48:49Z

src/main/java/com/serotonin/bacnet4j/npdu/ip/IpNetworkBuilder.java

+        this.broadcastAddress = IpNetworkUtils.getLocalBroadcastAddressString(localBindAddress);
+        this.subnetMask = IpNetworkUtils.getSubnetMask(localBindAddress);


If we are trying not to break existing usages of this code I think it would be wise to check if these fields are already set.
e.g.

if (this.broadcastAddress == null) { this.broadcastAddress = IpNetworkUtils.getLocalBroadcastAddressString(localBindAddress); } if (this.subnetMask == null) { this.subnetMask = IpNetworkUtils.getSubnetMask(localBindAddress); }

The operating systems would send the broadcast packets to the socket if and only if there is a socket opened for that specific broadcast address and that broadcast address & subnet mask is configured in the NIC. If the user happens to set it incorrectly, handling broadcast would become difficult. The same is applicable for local bind address.

Hence it would be right approach if the BACnet4J is initialized with the right set of network parameters. As per the above comments there are chances that the network could be built with wrong set of parameters. Hence checking of null would not be advisable. Further to avoid any confusion to the end user, building the network with NIC name is created. This method is simple and error free. For example

IpNetwork network = new IpNetworkBuilder().withInterfaceName("LAN1").build();

there are chances that the network could be built with wrong set of parameters.

Correct, but we have always provided the opportunity to configure this wrong. I am simply requesting that we maintain existing behavior and no not override properties which were explicitly configured by the programmer.

This has still not been addressed.

jazdw · 2025-01-10T17:49:32Z

src/main/java/com/serotonin/bacnet4j/npdu/ip/IpNetworkBuilder.java

@@ -60,7 +60,7 @@ public IpNetworkBuilder withLocalBindAddress(final String localBindAddress) {
    public IpNetworkBuilder withBroadcast(final String broadcastAddress, final int networkPrefixLength) {
        this.broadcastAddress = broadcastAddress;
        this.subnetMask = toIpAddrString(IpNetworkUtils.createMask(networkPrefixLength));
-
+        this.localBindAddress = IpNetworkUtils.getIPAddressString(this.broadcastAddress);


Check if localBindAddress is already set (as per above comment)

Same as above said response

jazdw · 2025-01-10T17:49:49Z

src/main/java/com/serotonin/bacnet4j/npdu/ip/IpNetworkBuilder.java

@@ -83,7 +83,7 @@ public IpNetworkBuilder withSubnet(final String subnetAddress, final int network
        final long subnet = IpNetworkUtils.bytesToLong(BACnetUtils.dottedStringToBytes(subnetAddress));

        this.broadcastAddress = toIpAddrString(subnet | negMask);
-
+        this.localBindAddress = IpNetworkUtils.getIPAddressString(this.broadcastAddress);


Check if localBindAddress is already set (as per above comment)

Same as above said response

src/test/java/com/serotonin/bacnet4j/adhoc/IpMasterTest.java

jazdw · 2025-01-10T18:04:06Z

src/test/java/com/serotonin/bacnet4j/npdu/ip/IpNetworkBuilderTest.java

+            assertEquals(IPAddress,builder.getLocalBindAddress());
+            assertEquals(interfaceDetails.get(interfaceIndex).get(2), builder.getBroadcastAddress());
+            assertEquals(interfaceDetails.get(interfaceIndex).get(3), builder.getSubnetMask());
+            System.out.println(IPAddress);


Remove System.out logging

jazdw · 2025-01-10T18:05:22Z

src/test/java/com/serotonin/bacnet4j/npdu/ip/IpNetworkBuilderTest.java

+            Random rand = new Random();
+            Map<Integer, List<String>> interfaceDetails = IpNetworkUtils.getLocalInterfaceAddresses();
+            List<Integer> keys = new ArrayList<>(interfaceDetails.keySet());
+            final int interfaceIndex = keys.get(rand.nextInt(keys.size()));


Repeated code between tests, move to a @Before method

Am I following this correctly - we are getting a random local interface address? This will make the test very unrepeatable.

The intent of the random is to verify that network can be built on any interface. Assume if there are 4 NIC, this test would choose any NIC randomly and verify the right parameters are set. Otherwise it can be hard-coded as 0 as well.

jazdw · 2025-01-10T18:06:31Z

src/test/java/com/serotonin/bacnet4j/npdu/ip/IpNetworkBuilderTest.java

+        }catch (final Exception e) {
+            // Should never happen, so just wrap in a RuntimeException
+            throw new RuntimeException(e);
+        }


Just add a throws declaration to the test method.

jazdw · 2025-01-10T18:06:50Z

src/test/java/com/serotonin/bacnet4j/npdu/ip/IpNetworkBuilderTest.java

+        Exception exception = assertThrows(RuntimeException.class,() -> {
+        final IpNetworkBuilder builder = new IpNetworkBuilder().withLocalBindAddress(IPAddress);
+    });


jazdw · 2025-01-16T15:02:53Z

src/main/java/com/serotonin/bacnet4j/npdu/ip/IpNetworkBuilder.java

+        this.broadcastAddress = IpNetworkUtils.getLocalBroadcastAddressString(localBindAddress);
+        this.subnetMask = IpNetworkUtils.getSubnetMask(localBindAddress);


there are chances that the network could be built with wrong set of parameters.

Correct, but we have always provided the opportunity to configure this wrong. I am simply requesting that we maintain existing behavior and no not override properties which were explicitly configured by the programmer.

jazdw · 2025-01-16T15:06:48Z

@kishorevenki you have not addressed the majority of my comments? Do you want to discuss?

…of List of strings. Improvement - Improve the methods in IpNetworkUtils.java Improvement - Improve the formatting of the comments using HTML List, parameter and return definition Improvement - Improve the build methods in IPMaster.java Improvement - pom.xml - Change the source and target java version to 21 from 1.8 to accommodate record type

jazdw

There are still comments which you have not addressed or discussed with me. Thank you for using the record though, it makes it much easier to follow.

jazdw · 2025-01-23T00:28:06Z

pom.xml

+					<source>21</source>
+					<target>21</target>


We definitely cannot target Java 21, since Mango only requires Java 17. We should discuss which version to target (I would be in favor of using Java 17).

jazdw · 2025-01-23T00:28:18Z

src/main/java/com/serotonin/bacnet4j/npdu/ip/IpNetworkBuilder.java

+        this.broadcastAddress = IpNetworkUtils.getLocalBroadcastAddressString(localBindAddress);
+        this.subnetMask = IpNetworkUtils.getSubnetMask(localBindAddress);


This has still not been addressed.

jazdw · 2025-01-23T00:29:00Z

src/main/java/com/serotonin/bacnet4j/npdu/ip/IpNetworkUtils.java

+    /**
+     * Convenient method to get the deails of Network Interface card
+     * <p>
+     * Values of the following parameters of each NIC are stored in record format:
+     * <ul>
+     * <li> index (int) - NIC Card Index assigned by the OS
+     * <li> Name of the NIC (String)
+     * <li> IP4 Address assigned to the NIC (String)
+     * <li> Broadcast Address of the NIC (String)
+     * <li> SubnetMask of the NIC calculated based on the NetworkPrefixLength (String)
+     * <li> Network Prefix Length of the NIC (int)
+     * </ul>
+     * @return the list of records of all NIC available in this device
+    */


A lot of these tags are not closed. Your IDE should be configured to warn about this.

jazdw · 2025-01-23T00:31:24Z

src/test/java/com/serotonin/bacnet4j/adhoc/IpMasterTest.java

-                .withBroadcast("255.255.255.255", 24)
+        Random rand = new Random();
+        List<IpNetworkUtils.InterfaceInfo> interfaceDetails = IpNetworkUtils.getLocalInterfaceAddresses();
+        final IpNetworkUtils.InterfaceInfo efaceInfo = interfaceDetails.get(rand.nextInt(interfaceDetails.size()));


Using a random real network interface makes this test not repeatable.

jazdw · 2025-01-23T00:33:17Z

src/test/java/com/serotonin/bacnet4j/npdu/ip/IpNetworkBuilderTest.java

+    @Before public void initialize(){
+        Random rand = new Random();
+        interfaceDetails = IpNetworkUtils.getLocalInterfaceAddresses();
+        efaceInfo = interfaceDetails.get(rand.nextInt(interfaceDetails.size()));  


Using a random real network interface makes this test not repeatable.

Also, lets not use the name efaceInfo, variable names should be understandable. Use interfaceInfo instead.

jazdw · 2025-01-23T00:35:00Z

src/main/java/com/serotonin/bacnet4j/npdu/ip/IpNetworkUtils.java

+     * <li> Integer format of the Network Prefix Length of the NIC </li> 
+     * </ul>
+     */
+    public static record InterfaceInfo(int index, 


Using static is redundant for records. Your IDE should warn about this.

jazdw · 2025-01-23T00:37:09Z

src/test/java/com/serotonin/bacnet4j/npdu/ip/IpNetworkBuilderTest.java

+        Exception exception = assertThrows(RuntimeException.class,() -> {
+        final IpNetworkBuilder builder = new IpNetworkBuilder().withLocalBindAddress(IPAddress);});
+        String actualMsg = exception.getMessage();
+        System.out.println(actualMsg);


Remove System.out logging

sonarqubecloud · 2025-01-28T18:36:41Z

Quality Gate passed

Issues
14 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

fix - test failure in IpNetworkBuilderTest.java improvement in comments incoporate review comments

kishorevenki requested review from jazdw, terrypacker and MertCingoz January 8, 2025 16:51

terrypacker reviewed Jan 10, 2025

View reviewed changes

src/main/java/com/serotonin/bacnet4j/npdu/ip/IpNetworkBuilder.java Outdated Show resolved Hide resolved

terrypacker reviewed Jan 10, 2025

View reviewed changes

src/test/java/com/serotonin/bacnet4j/adhoc/IpMasterTest.java Outdated Show resolved Hide resolved

jazdw requested changes Jan 10, 2025

View reviewed changes

Incorporated the changes as per the review comments.

88711bf

kishorevenki requested review from terrypacker and jazdw January 14, 2025 09:50

jazdw requested changes Jan 16, 2025

View reviewed changes

kishorevenki requested a review from jazdw January 21, 2025 14:10

jazdw requested changes Jan 23, 2025

View reviewed changes

jazdw reviewed Jan 23, 2025

View reviewed changes

fix - test failure in BBMDTest.java

83ac301

fix - test failure in IpNetworkBuilderTest.java improvement in comments incoporate review comments

kishorevenki requested a review from jazdw January 30, 2025 17:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MANGO-1817 - fix build methods in IpNetworkBuilder to get the Local IP Address, Broadcast Address and Subnet Mask #109

MANGO-1817 - fix build methods in IpNetworkBuilder to get the Local IP Address, Broadcast Address and Subnet Mask #109

kishorevenki commented Jan 8, 2025

terrypacker commented Jan 10, 2025

jazdw Jan 10, 2025

kishorevenki Jan 11, 2025

jazdw Jan 16, 2025

jazdw Jan 23, 2025

jazdw Jan 10, 2025

kishorevenki Jan 11, 2025

jazdw Jan 10, 2025

kishorevenki Jan 11, 2025

jazdw Jan 10, 2025

jazdw Jan 10, 2025

jazdw Jan 10, 2025

kishorevenki Jan 11, 2025

jazdw Jan 10, 2025

jazdw Jan 10, 2025

jazdw Jan 16, 2025

jazdw commented Jan 16, 2025

jazdw left a comment •

edited

Loading

jazdw Jan 23, 2025

jazdw Jan 23, 2025

jazdw Jan 23, 2025

jazdw Jan 23, 2025

jazdw Jan 23, 2025

jazdw Jan 23, 2025

jazdw Jan 23, 2025

jazdw Jan 23, 2025

sonarqubecloud bot commented Jan 28, 2025

		this.broadcastAddress = IpNetworkUtils.getLocalBroadcastAddressString(localBindAddress);
		this.subnetMask = IpNetworkUtils.getSubnetMask(localBindAddress);

MANGO-1817 - fix build methods in IpNetworkBuilder to get the Local IP Address, Broadcast Address and Subnet Mask #109

Are you sure you want to change the base?

MANGO-1817 - fix build methods in IpNetworkBuilder to get the Local IP Address, Broadcast Address and Subnet Mask #109

Conversation

kishorevenki commented Jan 8, 2025

terrypacker commented Jan 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jazdw commented Jan 16, 2025

jazdw left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 28, 2025

Quality Gate passed

jazdw left a comment •

edited

Loading