Handling pingrequests #46

hannastigland · 2024-02-09T07:44:21Z

Related Issue(s)

Closes #17

Proposed Changes

We have added functionality for pingrequests and pingresponse. Added class "PingHandler" with additional tests.

	🚀 This PR description was created by Ellipsis for commit `9cebb1c`.

Summary:

This PR adds support for handling MQTT PINGREQ messages and clean disconnects by introducing a new PingHandler class, updating the ClientHandler and PackageReader classes, and commenting out tests in the ClientHandlerTest class.

Key points:

Added functionality for handling MQTT PINGREQ messages and clean disconnects.
Updated PackageReader class to support clean disconnects.
Added PingHandler class with methods for handling MQTT PINGREQ messages.
Modified ClientHandler to use PingHandler for handling PINGREQ messages.
Commented out tests in ClientHandlerTest class.
Added and updated tests in PackageReaderTest and PingHandlerTest to cover new functionality.

Generated with ❤️ by ellipsis.dev

	🚀 This PR description was created by Ellipsis for commit `0c5c87f`.

Summary:

This PR introduces a PingHandler class for handling MQTT PINGREQ messages and clean disconnects, updates the ClientHandler and PackageReader classes, modifies the ClientHandlerTest class to cover the new functionality, and creates a new issue to address the issue with the tests in the ClientHandler class.

Key points:

Added PingHandler class for handling MQTT PINGREQ messages and clean disconnects.
Updated ClientHandler and PackageReader classes to support new functionality.
Commented out some tests in ClientHandlerTest due to issues.
Added and updated tests in ClientHandlerTest, PackageReaderTest and PingHandlerTest to cover new functionality.
Created a new issue (Test Coverage for ClientHandler with Updated Code #60 ) to address the issue with the tests in the ClientHandler class.

Generated with ❤️ by ellipsis.dev

…il.com>

…l@hotmail.com>

# Conflicts: # pom.xml

…il.com>

ellipsis-dev

Changes requested. Reviewed entire PR up to commit 3ecdb51

Reviewed 273 lines of code across 5 files in 1 minute(s) and 33 second(s).

See details

Skipped files: 0 (please contact us to request support for these files)
Confidence threshold: 50%
Drafted 0 additional comments.
Workflow ID: wflow_eXMs2Hr4Nvushi7L

Something look wrong? You can customize Ellipsis by editing the ellipsis.yaml for this repository.

Generated with ❤️ by ellipsis.dev

src/main/java/org/fungover/thunder/ClientHandler.java

ellipsis-dev · 2024-02-13T09:26:01Z

src/main/java/org/fungover/thunder/PackageReader.java


 public class PackageReader {
-    private final Map<InetAddress, Boolean> connectPackageSent = new HashMap<>();
+    public final Map<InetAddress, Boolean> connectPackageSent = new HashMap<>();


The 'connectPackageSent' field is public, which violates the principle of data encapsulation. Consider making this field private and provide public getter and setter methods if necessary.

ellipsis-dev · 2024-02-13T09:26:01Z

src/main/java/org/fungover/thunder/PingHandler.java

+public class PingHandler {
+    private static final Logger logger = Logger.getLogger(PingHandler.class.getName());
+
+    private PingHandler() {


This class appears to be a utility class, but it can still be instantiated. Consider making the class 'final' and throw an exception from the constructor to prevent instantiation.

Private constructor is fine by me, no need to throw exception :)

@BarbieQkiller Even if the constructor is private it's actually possible to use it with reflection. And if we look at Hibernate for example that can create instances of Entity classes or ValueObjects it doesn't care about the access modifier.
https://stackoverflow.com/questions/71824535/how-to-make-sure-instance-of-a-class-with-a-private-constructor-can-not-be-creat

That's why it can be a good practice in a bigger enterprise application to throw an exception from the private constructor in a utility class to prevent this happening without noticing.

ellipsis-dev · 2024-02-13T09:26:01Z

src/main/java/org/fungover/thunder/PingHandler.java

+        return bytesRead > 0 && buffer[0] == (byte) 0xC0;
+    }
+
+    public static Boolean sendPingResponse( OutputStream outputStream) throws IOException {


Instead of returning a Boolean value to indicate success or failure, consider letting this method throw an exception if the operation fails. This would be more idiomatic in Java.

src/test/java/org/fungover/thunder/PackageReaderTest.java

Win-ther

You should probably encapsulate the connectPackageSent-map again. And is there a reason for the removal of the isCleanDisconnect() tests?
Other than this, the code looks good.

src/main/java/org/fungover/thunder/PackageReader.java

src/test/java/org/fungover/thunder/PackageReaderTest.java

kappsegla · 2024-02-13T11:58:42Z

When running tests for this branch everything works but after merging main into this branch the following test starts failing:

at org.fungover.thunder.PackageReaderTest.shouldReturnFalseIfDoubleConnectMessageIsReceived(PackageReaderTest.java:143)

assertFalse(packageReader.isValidConnection(socketMock)); Fails.

kappsegla · 2024-02-13T12:59:15Z

@fungover/java23 Do we have any activity in fixing the comments from reviewers in this PR?

# Conflicts: # src/main/java/org/fungover/thunder/ClientHandler.java

BarbieQkiller · 2024-02-14T14:39:42Z

src/test/java/org/fungover/thunder/PingHandlerTest.java

+        assertTrue(PingHandler.isPingRequest(new byte[]{(byte) 0xC0},1));
+    }
+
+//    @Test


The code that is disabled by comments could be removed :)

Thank you for the feedback! Since the tests did not work previously, they were commented out. We have now updated the tests accordingly.

BarbieQkiller · 2024-02-14T14:42:03Z

src/main/java/org/fungover/thunder/PingHandler.java

+public class PingHandler {
+    private static final Logger logger = Logger.getLogger(PingHandler.class.getName());
+
+    private PingHandler() {


Private constructor is fine by me, no need to throw exception :)

altmannk

I guess you did a rollback? Heads up, I tried to run ClientHandlerTest in your branch but got stuck at the second test.

altmannk · 2024-02-14T15:21:09Z

src/main/java/org/fungover/thunder/ClientHandler.java

+                    System.out.println("Received MQTT PINGREQ message from client");
+                    PingHandler.sendPingResponse(outputStream);
+                } else {
+                    // Handle other types of messages


I guess this is because of a rollback?

Yes thank you for the feedback, ClienthHandler tests are currently not working, we commented the tests out and initiated a new issue (#60 ) for resolution

ellipsis-dev

Changes requested. Incremental review on commit 9cebb1c

Reviewed 115 lines of code across 2 files in 1 minute(s) and 22 second(s).

See details

Skipped files: 0
Confidence threshold: 50%
Drafted 0 additional comments.
Workflow ID: wflow_JoMTg7rEItBfXyvN

Something look wrong? You can customize Ellipsis by editing the ellipsis.yaml for this repository.

Generated with ❤️ by ellipsis.dev

ellipsis-dev · 2024-02-15T09:05:05Z

src/test/java/org/fungover/thunder/ClientHandlerTest.java

-
-        assertEquals(Collections.singletonList(socket2), clientHandler.getClients());
-    }
+//    @Test


The tests in this class have been commented out. Please uncomment them and ensure they pass with the new changes. If necessary, update the tests to reflect the changes in the code.

ellipsis-dev · 2024-02-15T09:05:05Z

src/main/java/org/fungover/thunder/PackageReader.java

    }

-    private static boolean isDisconnectPackage(int bytesRead, byte[] buffer) {
+    public static boolean isDisconnectPackage(byte[] buffer, int bytesRead) {


The order of parameters in the isDisconnectPackage method has been changed. Please ensure that all calls to this method have been updated to reflect this change.

sonarqubecloud · 2024-02-15T13:46:00Z

Quality Gate failed

Failed conditions
45.7% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

ellipsis-dev · 2024-02-15T13:47:59Z

src/test/java/org/fungover/thunder/ClientHandlerTest.java

-        clientHandler.handleConnection(socketMock);
-
-        assertEquals(0, clientHandler.getClients().size());
+        assertFalse(packageReader.isValidConnection(socketMock));


This test should be checking the client count, not the validity of the connection. Consider revising the test to check if the client count remains 0 after an invalid client tries to connect.

ellipsis-dev

Changes requested. Incremental review on commit 0c5c87f

Reviewed 128 lines of code across 1 files in 2 minute(s) and 43 second(s).

See details

Skipped files: 0
Confidence threshold: 50%
Drafted 1 additional comments.
Workflow ID: wflow_VZVS3mC58YComMUy

View 1 draft comments

These comments were drafted by Ellipsis, but were filtered out of the final review. They're included here so you can see our internal thought process and help you configure your ellipsis.yaml.

Drafted 1 additional comments

Comment at `src/test/java/org/fungover/thunder/ClientHandlerTest.java:44`

This test could be more comprehensive. Consider first checking if the client count increases after a client is added, and then check if it decreases after the client is disconnected.

Something look wrong? You can customize Ellipsis by editing the ellipsis.yaml for this repository.

Generated with ❤️ by ellipsis.dev

Win-ther

The function you wanted to implement works. Clients can send pinqrequests and can recieve pingresponse from the broker. But when you implemented this, you changed things in ClientHandler and ClientHandlerTest that caused our coverage in ClientHandler to go from 92% to 50%. This is probably something you should be able to change or revert so that the coverage becomes a bit better.

Rishad11

As far as I can tell, the code looks solid, and the tests cover essential scenarios.

hannastigland added 6 commits February 6, 2024 13:30

Changes by Martin

b652d7c

update pinghandler

7c9f235

update pinghandler Co-authored-by: Yrla Hackzell <yrla.hackzell@hotma…

059942c

…il.com>

add tests for PingHandler Co-authored-by: Yrla Hackzell <yrla.hackzel…

10b4fcb

…l@hotmail.com>

Merge branch 'main' into handling-pingrequests

af62350

# Conflicts: # pom.xml

update pinghandler Co-authored-by: Yrla Hackzell <yrla.hackzell@hotma…

a8590d4

…il.com>

hannastigland assigned yrlacornelia and hannastigland Feb 9, 2024

kappsegla added enhancement New feature or request minor labels Feb 9, 2024

kappsegla requested review from Frlundsten, TrungMa1 and Win-ther February 13, 2024 09:01

kappsegla marked this pull request as draft February 13, 2024 09:23

kappsegla marked this pull request as ready for review February 13, 2024 09:24

ellipsis-dev bot reviewed Feb 13, 2024

View reviewed changes

ellipsis-dev bot added a commit that referenced this pull request Feb 13, 2024

address comments left by @kappsegla on #46 (Handling pingrequests);

2a02d06

ellipsis-dev bot mentioned this pull request Feb 13, 2024

[Ellipsis] Refactor PackageReaderTest.java #56

Closed

Win-ther suggested changes Feb 13, 2024

View reviewed changes

src/main/java/org/fungover/thunder/PackageReader.java Outdated Show resolved Hide resolved

src/test/java/org/fungover/thunder/PackageReaderTest.java Show resolved Hide resolved

hannastigland removed request for Frlundsten and TrungMa1 February 13, 2024 15:56

kappsegla enabled auto-merge February 14, 2024 12:37

hannastigland added 2 commits February 14, 2024 14:42

Merge remote-tracking branch 'origin/main' into handling-pingrequests

13903ad

# Conflicts: # src/main/java/org/fungover/thunder/ClientHandler.java

Update

8a87230

kappsegla disabled auto-merge February 14, 2024 14:05

kappsegla enabled auto-merge February 14, 2024 14:05

hannastigland force-pushed the handling-pingrequests branch from 3ecdb51 to 8a87230 Compare February 14, 2024 14:06

updated tests Co-authored-by: Yrla Hackzell <yrla.hackzell@hotmail.com>

43cf68a

BarbieQkiller reviewed Feb 14, 2024

View reviewed changes

altmannk reviewed Feb 14, 2024

View reviewed changes

small updates Co-authored-by: Yrla Hackzell <yrla.hackzell@hotmail.com>

9cebb1c

ellipsis-dev bot reviewed Feb 15, 2024

View reviewed changes

Win-ther mentioned this pull request Feb 15, 2024

Test Coverage for ClientHandler with Updated Code #60

Closed

updated tests Co-authored-by: Yrla Hackzell <yrla.hackzell@hotmail.com>

0c5c87f

ellipsis-dev bot reviewed Feb 15, 2024

View reviewed changes

Win-ther suggested changes Feb 16, 2024

View reviewed changes

Rishad11 approved these changes Mar 3, 2024

View reviewed changes

Handling pingrequests #46

Are you sure you want to change the base?

Handling pingrequests #46

Uh oh!

Conversation

hannastigland commented Feb 9, 2024 • edited by ellipsis-dev bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related Issue(s)

Proposed Changes

Summary:

Summary:

Uh oh!

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ellipsis-dev bot Feb 13, 2024

Choose a reason for hiding this comment

Uh oh!

ellipsis-dev bot Feb 13, 2024

Choose a reason for hiding this comment

Uh oh!

BarbieQkiller Feb 14, 2024

Choose a reason for hiding this comment

Uh oh!

kappsegla Feb 14, 2024

Choose a reason for hiding this comment

Uh oh!

ellipsis-dev bot Feb 13, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Win-ther left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kappsegla commented Feb 13, 2024

Uh oh!

kappsegla commented Feb 13, 2024

Uh oh!

BarbieQkiller Feb 14, 2024

Choose a reason for hiding this comment

Uh oh!

hannastigland Feb 15, 2024

Choose a reason for hiding this comment

Uh oh!

BarbieQkiller Feb 14, 2024

Choose a reason for hiding this comment

Uh oh!

altmannk left a comment

Choose a reason for hiding this comment

Uh oh!

altmannk Feb 14, 2024

Choose a reason for hiding this comment

Uh oh!

hannastigland Feb 15, 2024

Choose a reason for hiding this comment

Uh oh!

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

Uh oh!

ellipsis-dev bot Feb 15, 2024

Choose a reason for hiding this comment

Uh oh!

ellipsis-dev bot Feb 15, 2024

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Feb 15, 2024

Quality Gate failed

Uh oh!

ellipsis-dev bot Feb 15, 2024

Choose a reason for hiding this comment

Uh oh!

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

Drafted 1 additional comments

Comment at src/test/java/org/fungover/thunder/ClientHandlerTest.java:44

Uh oh!

Win-ther left a comment

hannastigland commented Feb 9, 2024 •

edited by ellipsis-dev bot

Loading

Comment at `src/test/java/org/fungover/thunder/ClientHandlerTest.java:44`