paranext-core 61: Pair requests to their respective responses

[lyonsil]

This change is 

[FoolRunning]

Reviewed 22 of 22 files at r1, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @lyonsil and @tjcouch-sil)

---

c-sharp/Data/NetworkConnectorTypes.cs line 7 at r1 (raw file):

// **********************************************************************************************
// NOTE: The types defined in this file should match what is defined in NetworkConnectorTypes.ts

Since you have split this file up, it no longer mimics what is in NetworkConnectorTypes.ts. I feel like this might be a downside to your refactoring. There is no longer a single location on both ends that can be searched for parity (i.e. NetworkConnectorTypes.cs and NetworkConnectorTypes.ts). The problem is that the types in both must match as much as possible so they can be serialized/deserialized properly.

---

c-sharp/MessageHandlers/MessageHandlerRequestByRequestType.cs line 38 at r1 (raw file):

                else
                    _messageSink.SendMessage(new MessageResponse(0, request.RequestType, request.RequestId, request.SenderId, response.ErrorMessage));
            }

At the very least, I think we should log that there was a request that we didn't handle (if _handlersByRequestType doesn't contain the item). The server shouldn't be sending us requests that we didn't register to handle.

---

c-sharp/Messages/MessageInitClient.cs line 10 at r1 (raw file):

public sealed class MessageInitClient : Message
{
    public sealed class NetworkConnectorInfo

Is there a particular reason you embedded this class inside of MessageInitClient? That means that anywhere it's accessed we need to prepend MessageInitClient to it. In general, we should avoid public classes inside of another class. I think it should be moved out of this class (ideally in the same file in a region).
Also, it seems to go against our coding standard (specifically this part ).

---

src/shared/data/NetworkConnectorTypes.ts line 2 at r1 (raw file):

// **********************************************************************************************
// NOTE: The types defined in this file should match what is defined in NetworkConnectorTypes.cs

See my previous comment. This code comment is no longer relevant. Something should be done.

---

c-sharp/MessageTransports/PapiClient.cs line 111 at r1 (raw file):

            _clientId,
            RequestType.RegisterRequest,
            Interlocked.Increment(ref _nextRequestId),

This interlock shouldn't ever be needed since request registrations should only be done once at the start and awaited, but I guess it's better to be prepared.

---

c-sharp/MessageTransports/PapiClient.cs line 124 at r1 (raw file):

                {
                    var responder = _messageHandlersByMessageType[MessageType.Request];
                    if (responder == null)

Using the indexer on a Dictionary will throw an exception if not found, it will not return null. If you want to check if it exists, you need to use TryGetValue(). In this case, however, since the item is added to the dictionary in the constructor, I would remove the check for null completely since it should not ever happen.

---

c-sharp/MessageTransports/PapiClient.cs line 136 at r1 (raw file):

                    else
                    {
                        Console.Error.WriteLine("Unexpected message handler registered for MessageType.Request");

Similarly, since the handler is registered in the constructor, it had better be of the type MessageHandlerRequestByRequestType so you should just do a direct cast instead of checking it (i.e. allow it to throw an exception sooner instead of failing in another way later).

---

c-sharp/MessageTransports/PapiClient.cs line 215 at r1 (raw file):

    /// Sends the specified message to the server
    /// </summary>
    public async Task SendMessage(Message message)

Why was this made public? Ideally, PapiClient should be the only thing creating Messages and sending them to the server.

---

c-sharp/MessageTransports/PapiClient.cs line 220 at r1 (raw file):

            throw new InvalidOperationException("Can not send data when the socket is closed");

        message.SenderId = _clientId;

The sender is required in the Message constructor. This should be redundant.
If wanted, this could be turned into Debug.Assert(message.SenderId == _clientId) to make sure that the messages are created correctly.

[lyonsil]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @FoolRunning and @tjcouch-sil)

---

c-sharp/Data/NetworkConnectorTypes.cs line 7 at r1 (raw file):

Previously, FoolRunning (Tim Steenwyk) wrote…

Since you have split this file up, it no longer mimics what is in NetworkConnectorTypes.ts. I feel like this might be a downside to your refactoring. There is no longer a single location on both ends that can be searched for parity (i.e. NetworkConnectorTypes.cs and NetworkConnectorTypes.ts). The problem is that the types in both must match as much as possible so they can be serialized/deserialized properly.

Yes, that is a downside to my refactoring. Even if I back it out doing it was helpful for me to get a better sense of how everything fits together. Keeping 9+ classes in a single file isn't something I've normally seen done, and it makes glancing at the file structure no longer a good parallel to understanding the class hierarchy. It was hard for me to understand what was going on at first with multiple classes being held together like that. Even the current arrangement isn't foolproof as there was a new message type in typescript that wasn't in C#. It actually made it more visible to me when I broke down the file because I could more easily recognize what was in C# and what wasn't.

---

c-sharp/MessageHandlers/MessageHandlerRequestByRequestType.cs line 38 at r1 (raw file):

Previously, FoolRunning (Tim Steenwyk) wrote…

At the very least, I think we should log that there was a request that we didn't handle (if _handlersByRequestType doesn't contain the item). The server shouldn't be sending us requests that we didn't register to handle.

That is a good point. I will make the change.

---

c-sharp/Messages/MessageInitClient.cs line 10 at r1 (raw file):

Previously, FoolRunning (Tim Steenwyk) wrote…

Is there a particular reason you embedded this class inside of MessageInitClient? That means that anywhere it's accessed we need to prepend MessageInitClient to it. In general, we should avoid public classes inside of another class. I think it should be moved out of this class (ideally in the same file in a region).
Also, it seems to go against our coding standard (specifically this part ).

The reason I did it was that NetworkConnectorInfo only ever has any meaning within this message. It is never used in any other context. Having said that, I will back it out. I don't feel strongly about this, and I was focused on the one "green check" part of the guidelines (DO use nested types when the relationship between the nested type and its outer type is such that member-accessibility semantics are desirable). There are good reasons to not do it, too.

---

src/shared/data/NetworkConnectorTypes.ts line 2 at r1 (raw file):

Previously, FoolRunning (Tim Steenwyk) wrote…

See my previous comment. This code comment is no longer relevant. Something should be done.

Understood

---

c-sharp/MessageTransports/PapiClient.cs line 111 at r1 (raw file):

Previously, FoolRunning (Tim Steenwyk) wrote…

This interlock shouldn't ever be needed since request registrations should only be done once at the start and awaited, but I guess it's better to be prepared.

Yeah this was done in part based on questions I asked the team at Wednesday's scrum (I think). I was trying to get a sense of what the intended use cases were/could be. I think this is more future proofing or implying that we expect this could be used in a more dynamic way in the future when, for example, the code is used in a live website.

---

c-sharp/MessageTransports/PapiClient.cs line 124 at r1 (raw file):

Previously, FoolRunning (Tim Steenwyk) wrote…

Using the indexer on a Dictionary will throw an exception if not found, it will not return null. If you want to check if it exists, you need to use TryGetValue(). In this case, however, since the item is added to the dictionary in the constructor, I would remove the check for null completely since it should not ever happen.

Sure, I can remove the null check here.

---

c-sharp/MessageTransports/PapiClient.cs line 136 at r1 (raw file):

Previously, FoolRunning (Tim Steenwyk) wrote…

Similarly, since the handler is registered in the constructor, it had better be of the type MessageHandlerRequestByRequestType so you should just do a direct cast instead of checking it (i.e. allow it to throw an exception sooner instead of failing in another way later).

This was done with defensive coding in mind. That is, what if someone in the future updates one part of the code and doesn't update this later? It seems like it would be nice to allow the process to keep going. In this case the registration will fail gracefully which seems like the desired outcome.

Having said this, this is a pretty minor thing that would be pretty obvious if it broke, so I will remove it.

---

c-sharp/MessageTransports/PapiClient.cs line 215 at r1 (raw file):

Previously, FoolRunning (Tim Steenwyk) wrote…

Why was this made public? Ideally, PapiClient should be the only thing creating Messages and sending them to the server.

Well this was actually pretty important in terms of trying to keep logic localized to different parts of the code. I was trying to allow all IMessageHandler classes to own the logic they needed and keep them out of PapiClient. Request handlers in particular need to send responses based on the messages they received. Having said that, I'm realizing now that I could change IMessageHandler to return an optional Message itself, and PapiClient could send that message on behalf of message handlers in that case. So I can change this back to private and get rid of IMessageSink at the same time.

---

c-sharp/MessageTransports/PapiClient.cs line 220 at r1 (raw file):

Previously, FoolRunning (Tim Steenwyk) wrote…

The sender is required in the Message constructor. This should be redundant.
If wanted, this could be turned into Debug.Assert(message.SenderId == _clientId) to make sure that the messages are created correctly.

I would argue that things that create messages should not need to know the value of the client ID. The entity that owns the client ID is PapiClient, and PapiClient really should not need to expose that to anyone else. I actually wondered if it would be better to not include the client ID in constructors in general, but I wasn't sure if that would break JSON deserialization, so I left it as-is. I do think it's actually the right thing for PapiClient to overwrite any client ID in messages before being sent, though.

[lyonsil]

Reviewable status: 9 of 23 files reviewed, 8 unresolved discussions (waiting on @FoolRunning and @tjcouch-sil)

---

c-sharp/MessageHandlers/MessageHandlerRequestByRequestType.cs line 38 at r1 (raw file):

Previously, lyonsil wrote…

That is a good point. I will make the change.

Done

---

c-sharp/Messages/MessageInitClient.cs line 10 at r1 (raw file):

Previously, lyonsil wrote…

The reason I did it was that NetworkConnectorInfo only ever has any meaning within this message. It is never used in any other context. Having said that, I will back it out. I don't feel strongly about this, and I was focused on the one "green check" part of the guidelines (DO use nested types when the relationship between the nested type and its outer type is such that member-accessibility semantics are desirable). There are good reasons to not do it, too.

Done

---

c-sharp/MessageTransports/PapiClient.cs line 124 at r1 (raw file):

Previously, lyonsil wrote…

Sure, I can remove the null check here.

Done

---

c-sharp/MessageTransports/PapiClient.cs line 136 at r1 (raw file):

Previously, lyonsil wrote…

This was done with defensive coding in mind. That is, what if someone in the future updates one part of the code and doesn't update this later? It seems like it would be nice to allow the process to keep going. In this case the registration will fail gracefully which seems like the desired outcome.

Having said this, this is a pretty minor thing that would be pretty obvious if it broke, so I will remove it.

Done

---

c-sharp/MessageTransports/PapiClient.cs line 215 at r1 (raw file):

Previously, lyonsil wrote…

Well this was actually pretty important in terms of trying to keep logic localized to different parts of the code. I was trying to allow all IMessageHandler classes to own the logic they needed and keep them out of PapiClient. Request handlers in particular need to send responses based on the messages they received. Having said that, I'm realizing now that I could change IMessageHandler to return an optional Message itself, and PapiClient could send that message on behalf of message handlers in that case. So I can change this back to private and get rid of IMessageSink at the same time.

Done
I updated IMessageHandler as mentioned above.

---

c-sharp/MessageTransports/PapiClient.cs line 220 at r1 (raw file):

Previously, lyonsil wrote…

I would argue that things that create messages should not need to know the value of the client ID. The entity that owns the client ID is PapiClient, and PapiClient really should not need to expose that to anyone else. I actually wondered if it would be better to not include the client ID in constructors in general, but I wasn't sure if that would break JSON deserialization, so I left it as-is. I do think it's actually the right thing for PapiClient to overwrite any client ID in messages before being sent, though.

I actually went back and removed the client ID from all the message constructors except MessageClientConnect and MessageInitClient since it is integral for the purpose of those two messages. The other message creators shouldn't necessarily have to know client IDs ahead of creating a message. PapiClient owns the connection and tracking of that ID.

[FoolRunning]

Reviewed 14 of 14 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @irahopkinson, @lyonsil, and @tjcouch-sil)

---

c-sharp/Messages/MessageEvent.cs line 28 at r2 (raw file):

    public override string ToString()
    {
        return $"Event: {EventType} from {SenderId}";

I'm guessing these changes were to try make it more in line with what is going on in the TS. However, I think this was inadequate because of the /** Contents of the event */ event: T; in the TS (see InternalEvent). I think there is a task to update the C# for the changes to the TS already (or there will be), so it might be best to just revert these changes and leave updating the API to a later task.

---

c-sharp/Messages/MessageInitClient.cs line 10 at r1 (raw file):

Previously, lyonsil wrote…

Done

I don't prefer having the class in its own file when it feels like it belongs with MessageInitClient, but that's probably just personal preference so, for now, I'll drop the suggestion to put it into the same file.

---

c-sharp/Messages/MessageType.cs line 7 at r1 (raw file):

// **********************************************************************************************
// NOTE: The types defined in this file should match what is defined in NetworkConnectorTypes.ts
// **********************************************************************************************

I'm not sure that completely removing this comment without doing something else was best, but I'm not sure what is best at this point, so let's leave it for now.

---

c-sharp/MessageTransports/PapiClient.cs line 136 at r1 (raw file):

Previously, lyonsil wrote…

Done

My philosophy is to fail sooner rather than have something fail later and you need to track it down. Thus, to me, it's better to have a direct cast and throw an exception when it goes wrong than just silently fail (even with a console message).

---

c-sharp/MessageTransports/PapiClient.cs line 220 at r1 (raw file):

Previously, lyonsil wrote…

I actually went back and removed the client ID from all the message constructors except MessageClientConnect and MessageInitClient since it is integral for the purpose of those two messages. The other message creators shouldn't necessarily have to know client IDs ahead of creating a message. PapiClient owns the connection and tracking of that ID.

I think that's fine for now. I think it might be best at some point to make everything consistent (i.e. don't have any message creators ever have to worry about the client ID).

[lyonsil]

Reviewable status: 22 of 24 files reviewed, 1 unresolved discussion (waiting on @FoolRunning, @irahopkinson, and @tjcouch-sil)

---

c-sharp/Messages/MessageEvent.cs line 28 at r2 (raw file):

Previously, FoolRunning (Tim Steenwyk) wrote…

I'm guessing these changes were to try make it more in line with what is going on in the TS. However, I think this was inadequate because of the /** Contents of the event */ event: T; in the TS (see InternalEvent). I think there is a task to update the C# for the changes to the TS already (or there will be), so it might be best to just revert these changes and leave updating the API to a later task.

I was trying to get something to at least show proper console output when events are received. What I had before wasn't actually showing anything meaningful from inside the event. I was basically misunderstanding what the JSON deserializer was doing. I didn't try to align to anything in TS other than the fact that I see eventType exists as a field both in the code and in the payloads I've been getting. A more thorough updating of the message is still in order.

---

c-sharp/Messages/MessageType.cs line 7 at r1 (raw file):

Previously, FoolRunning (Tim Steenwyk) wrote…

I'm not sure that completely removing this comment without doing something else was best, but I'm not sure what is best at this point, so let's leave it for now.

I just added a README.md and adjusted text in TS to try to help with this.

[irahopkinson]

Reviewed 8 of 22 files at r1, 14 of 14 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @FoolRunning, @lyonsil, and @tjcouch-sil)

---

c-sharp/MessageHandlers/MessageHandlerRequestByRequestType.cs line 25 at r3 (raw file):

            var request = (MessageRequest)message;
            if (_handlersByRequestType.TryGetValue(request.RequestType, out Func<dynamic, ResponseToRequest>? handler))

Use nominal flow (not indented by if or else) only dealing with exceptions in the if with no else, i.e:

            if (!_handlersByRequestType.TryGetValue(request.RequestType, out Func<dynamic, ResponseToRequest>? handler))
            {
                Console.Error.WriteLine("Unable to process unexpected request type: {0}", request.RequestType);
                return null;
            }

            var response = handler(request.Contents);
            if (response.Success)
                return new MessageResponse(request.RequestType, request.RequestId, request.SenderId, response.Contents);
            else
                return new MessageResponse(request.RequestType, request.RequestId, request.SenderId, response.ErrorMessage);

---

c-sharp/MessageTransports/PapiClient.cs line 213 at r3 (raw file):

        message.SenderId = _clientId;
        string jsonData = JsonSerializer.Serialize(message, s_serializationOptions);
        Console.WriteLine("Sending message over websocket: {0}", jsonData);

I'm wondering if we really want to be spitting all this to the logs (also L249 below). For one it will make the logs fuller. Also we may be spitting user sensative data to the logs. We probably need to talk about how we handle this in general.

[FoolRunning]

Reviewed all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @lyonsil and @tjcouch-sil)

[lyonsil]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @irahopkinson and @tjcouch-sil)

---

c-sharp/MessageHandlers/MessageHandlerRequestByRequestType.cs line 25 at r3 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Use nominal flow (not indented by if or else) only dealing with exceptions in the if with no else, i.e:

            if (!_handlersByRequestType.TryGetValue(request.RequestType, out Func<dynamic, ResponseToRequest>? handler))
            {
                Console.Error.WriteLine("Unable to process unexpected request type: {0}", request.RequestType);
                return null;
            }

            var response = handler(request.Contents);
            if (response.Success)
                return new MessageResponse(request.RequestType, request.RequestId, request.SenderId, response.Contents);
            else
                return new MessageResponse(request.RequestType, request.RequestId, request.SenderId, response.ErrorMessage);

Sure, I will make this change. I plan to recreate this PR on top of your C# formatting using a new branch to avoid what would be (at least for me) a painful rebase, and I will do it there.

---

c-sharp/MessageTransports/PapiClient.cs line 213 at r3 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

I'm wondering if we really want to be spitting all this to the logs (also L249 below). For one it will make the logs fuller. Also we may be spitting user sensative data to the logs. We probably need to talk about how we handle this in general.

I found this logging super helpful from a debugging standpoint, but I agree that in the normal course of things we would not want to log this much out. This is a prime candidate for using log levels IMO. I would expect this to be at a level of verbosity that is not logging by default. For example, if there are 5 levels (i.e., ERROR, WARN, INFO, DEBUG, TRACE), I could see this being DEBUG or TRACE with the default logging set to WARN (or arguably ERROR or INFO).

Until there are log levels in place or we find this spews far too much data for our own debugging, I would like to leave this here.

[lyonsil]

Comments have been folded into a new branch and PR that is on top of Ira's latest updates in main. This was handled as a new branch/PR to avoid rebasing pain.