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

feat: added WebSocket support in AtLookup #709

Closed
wants to merge 30 commits into from

Conversation

purnimavenkatasubbu
Copy link
Member

@purnimavenkatasubbu purnimavenkatasubbu commented Nov 13, 2024

- What I did
Implemented WebSocket support by introducing a new connection factory structure to support both SecureSocket and WebSocket connections. This update allows AtLookupImpl to dynamically choose between secure socket and WebSocket connections, enhancing flexibility and enabling WebSocket-based communication when required.

- How I did it

  • Created a new abstract factory class, AtLookupOutboundConnectionFactory, to standardize the creation of underlying connections, outbound connection wrappers, and message listeners for different connection types.
  • Developed AtLookupSecureSocketFactory and AtLookupWebSocketFactory to handle secure socket and WebSocket connections, respectively. Each factory is responsible for:
    • Establishing the underlying connection (either SecureSocket or WebSocket).
    • Wrapping the connection into an outbound connection instance (OutboundConnectionImpl or OutboundWebsocketConnectionImpl).

- How to verify it

  • Existing tests for SecureSocket connections should continue working as expected.
  • Added functional tests by passing the AtLookupWebSocketFactory should pass

- Description for the changelog

add WebSocket support in AtLookupImpl

@purnimavenkatasubbu purnimavenkatasubbu marked this pull request as ready for review November 14, 2024 12:52
@@ -240,22 +243,24 @@ class AtLookupImpl implements AtLookUp {

Future<void> createConnection() async {
if (!isConnectionAvailable()) {
if (_connection != null) {
if (connection != null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see connection and _connection both..can we use just _connection while referring the var inside AtLookUpImpl

Copy link
Member Author

@purnimavenkatasubbu purnimavenkatasubbu Nov 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated be8058b

@@ -6,9 +6,12 @@ library at_lookup;
export 'src/at_lookup.dart';
export 'src/at_lookup_impl.dart';
export 'src/connection/outbound_connection.dart';
export 'src/connection/outbound_websocket_connection.dart';
export 'src/connection/outbound_connection_impl.dart';
export 'src/exception/at_lookup_exception.dart';
export 'src/monitor_client.dart';
export 'src/cache/secondary_address_finder.dart';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we have modified SecondaryUrlFinder constructor and exported this class, the change should be treated as breaking and have a major release

Copy link
Member Author

@purnimavenkatasubbu purnimavenkatasubbu Nov 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated be8058b

.atLookupSocketListenerFactory(outboundConnection);

// Set the connection type in `_webSocketConnection` or `_connection`
if (underlying is WebSocket) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this type casting has no impact on the code since we are not accessing any attributes specific to OutboundWebSocketConnection or OutboundConnection
If _connection is not set before, we can just have
_connection = outboundConnection

Copy link
Member Author

@purnimavenkatasubbu purnimavenkatasubbu Nov 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated [be8058b]

export 'src/connection/outbound_connection_impl.dart';
export 'src/exception/at_lookup_exception.dart';
export 'src/monitor_client.dart';
export 'src/cache/secondary_address_finder.dart';
export 'src/cache/cacheable_secondary_address_finder.dart';
export 'src/util/secure_socket_util.dart';
export 'src/connection/outbound_websocket_connection_impl.dart';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please avoid exporting the implementation classes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

export 'src/connection/outbound_connection_impl.dart';
export 'src/exception/at_lookup_exception.dart';
export 'src/monitor_client.dart';
export 'src/cache/secondary_address_finder.dart';
export 'src/cache/cacheable_secondary_address_finder.dart';
export 'src/util/secure_socket_util.dart';
// export 'src/connection/outbound_websocket_connection_impl.dart';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest removing the line instead of commenting it out.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, My Bad!! Removed commented line

@@ -661,7 +684,7 @@ class AtLookupImpl implements AtLookUp {
Future<void> _sendCommand(String command) async {
await createConnection();
logger.finer('SENDING: $command');
await _connection!.write(command);
_connection!.write(command);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

await is removed here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think await is needed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I verified the code and "write" operation on the socket does not return a future. Hence suggested to remove await.

serverSide: false,
);

print('WebSocket connection established');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this print and replace with logger

@purnimavenkatasubbu
Copy link
Member Author

Opened a new PR with the refactored code. Hence closing this PR

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.

5 participants