Make `clientName` and `clientVersion` get-only protocol requirements #954

designatednerd · 2019-12-16T23:17:48Z

BREAKING CHANGE

This has been much-discussed in #877 and #917, and it's clear that for people implementing their own NetworkTransport method rather than using one of ours, having to provide a setter for this is really annoying. If implementers still want to make the property get/set, they can (and we do with HTTPNetworkTransport).

Also added a convenience method to add the headers to any URLRequest, and added a note to the send method that these should probably be sent along if you want to use monitoring tools like Apollo Graph Manager.

…nsport

…d that

designatednerd · 2019-12-16T23:18:28Z

@bwhiteley @cysp

gsabran · 2019-12-16T23:42:05Z

Thanks @designatednerd for putting this PR out so quickly.

IMO those requirements should be totally removed from the protocol. I think that folks using custom implementations are doing so because they want to be in control, and are accepting the corresponding responsibilities. I see protocols as describing the requirements that Apollo's SDK needs to work properly, and not as guiding the external implementation.

It gives me the inaccurate impression that the Apollo SDK needs to read those values to work properly, while the documentation of send<Operation> is actually what matters most in this case.

Sources/Apollo/NetworkTransport.swift

designatednerd · 2019-12-16T23:54:21Z

It gives me the inaccurate impression that the Apollo needs to read those values to work properly

The iOS SDK doesn't, but Apollo Graph Manager does (as would basically anything else that's trying to look at what version/client are sending a particular request).

It's certainly a best practice to include these in some way, shape, or form so that anything looking at where requests are coming from is able to quickly tell what's coming from what version of what client. I would like to steer people real hard towards following this best practice, and making them at least implement these properties, even if they want to send them with other headers.

designatednerd · 2019-12-17T03:50:02Z

@gsabran Better?

gsabran · 2019-12-17T17:26:07Z

👍Ok thanks!

designatednerd · 2019-12-17T20:40:42Z

@bwhiteley @cysp any further thoughts? I'll leave this open until tomorrow morning for more comments.

cysp · 2019-12-18T22:28:34Z

As an implementor of a custom NetworkTransport, my mind didn't make the connection from "the NetworkTransport protocol requires me to implement some client identification properties, I assumed that the properties were going to be read (and set) by somewhere else in the iOS SDK. The comment added to send() here indicating that it's actually the network transport itself which is supposed to use them seems like it might be helpful in that regard. 🙂

We don't use the graph manager (though we're interested in its capabilities and will be thinking more about it "soon"), but perhaps a dashboard on there with a list of user-agents from which it has seen queries without client identification would be a good way to remind implementors to supply the client identification headers?

Now that these properties are get-only requirements, would it be worth giving them default implementations that forward through to the defaultClient(Name|Version) static properties?

networktransport-default-client-vars-light.diff

diff --git c/Sources/Apollo/NetworkTransport.swift i/Sources/Apollo/NetworkTransport.swift
index 63a90fe3..766944fc 100644
--- c/Sources/Apollo/NetworkTransport.swift
+++ i/Sources/Apollo/NetworkTransport.swift
@@ -41,6 +41,10 @@ public extension NetworkTransport {
     return "\(identifier)-apollo-ios"
   }
   
+  var clientName: String {
+    return Self.defaultClientName
+  }
+
   /// The default client version to use when setting up the `clientVersion` property.
   static var defaultClientVersion: String {
     var version = String()
@@ -62,6 +66,10 @@ public extension NetworkTransport {
     
     return version
   }
+
+  var clientVersion: String {
+    return Self.defaultClientVersion
+  }
   
   /// Adds the Apollo client headers for this instance of `NetworkTransport` to the given request
   /// - Parameter request: A mutable URLRequest to add the headers to.

designatednerd · 2019-12-18T23:59:54Z

Now that these properties are get-only requirements, would it be worth giving them default implementations that forward through to the defaultClient(Name|Version) static properties?

I can't believe I didn't think of this (I think because I was still thinking about the "override to get/set" situation), this is an excellent idea.

cysp · 2019-12-19T03:06:11Z

Sources/Apollo/NetworkTransport.swift

-  /// The header field name for the Client Name
-  static var headerFieldNameClientName: String {
+  /// The field name for the Apollo Client Name header
+  var headerFieldNameApolloClientName: String {


I have two questions about these header name properties:

what was the reason that these changed to be instance variables, rather than static variables? (since addApolloClientHeaders() is in a generic context, I think it has access to Self to access the static properties, it certainly does with newer versions of Swift)

at the moment these fields are not protocol requirements, so despite the fact that addApolloClientHeaders() accesses them through self, because that's happening in a generic context the NetworkTransport default implementation is used and any specialisation in an implementation is ignored, is that intentional? if that is intentional (and it would make sense to me that it is, because why would implementors want to customise the header field names?) it would be less surprising (at least to me) when reading the code if those were just static variables somewhere, not accessed through the protocol. 🙃

So there's a couple thoughts happening here:

It seemed goofy to me that at the call site you had to specify the class name of the current class, although that's less bad with being able to call Self within the protocol (I thought the Self outside of the protocol thing hadn't landed yet?), and there was no real reason for them to be static rather than instance other than it seemed good for namespacing. However, now that by default they're mostly accessed within the protocol, I'll rethink this change.

This is certainly in the realm of "my opinion" vs "the right way to do things", but I hate free-floating static variables. It's almost always primarily related to something, so I think it's way better to have static variables on whatever it is they're related to rather than floating in space.

Cool, namespacing is good.
From reading the code I was unsure whether it was intentional that implementations of NetworkTransport were not capable of specialising those properties because I saw them being accessed statically on implementation types:
/~https://github.com/apollographql/apollo-ios/search?q=headerFieldNameClientName&unscoped_q=headerFieldNameClientName

🙃

Sources/ApolloWebSocket/SplitNetworkTransport.swift

cysp · 2019-12-19T04:25:08Z

Sources/Apollo/NetworkTransport.swift

-  /// The header field name for the Client Name
-  static var headerFieldNameClientName: String {
+  /// The field name for the Apollo Client Name header
+  var headerFieldNameApolloClientName: String {


Cool, namespacing is good.
From reading the code I was unsure whether it was intentional that implementations of NetworkTransport were not capable of specialising those properties because I saw them being accessed statically on implementation types:
/~https://github.com/apollographql/apollo-ios/search?q=headerFieldNameClientName&unscoped_q=headerFieldNameClientName

🙃

bwhiteley · 2019-12-19T05:29:18Z

Now that these properties are get-only requirements, would it be worth giving them default implementations that forward through to the defaultClient(Name|Version) static properties?

I can't believe I didn't think of this (I think because I was still thinking about the "override to get/set" situation), this is an excellent idea.

#917 (comment)
;)

bwhiteley · 2019-12-19T05:40:06Z

I too somehow missed that it's up to an implementation of NetworkTransport to actually include these headers in the request.

bwhiteley · 2019-12-19T05:54:53Z

It's certainly a best practice to include these in some way, shape, or form so that anything looking at where requests are coming from is able to quickly tell what's coming from what version of what client. I would like to steer people real hard towards following this best practice, and making them at least implement these properties, even if they want to send them with other headers.

Are you saying it doesn't have to be these particular headers? Is this what User-Agent is typically used for?

bwhiteley · 2019-12-19T06:27:15Z

Ultimately I agree with @gsabran now that I understand better what's going on here. This is basically some documentation and a convenience function.

However, by changing the properties to get-only and providing default implementations, the properties no longer impose an undue burden on custom implementations of NetworkTransport. As a result, these are solid improvements.

Co-Authored-By: Scott Talbot <stalbot@scentregroup.com>

designatednerd · 2019-12-19T18:02:56Z

It doesn't have to be these particular headers if you're not using Apollo Graph Manager. It does if you're using Apollo graph manager. Given that the library is owned and maintained by Apollo, we're certainly going to do everything we can to make it easy for people to use Graph Manager. 😇

User-Agent could theoretically be used for this, but separating client and version out in the same string is annoying, and having them be separate is really helpful for diagnostics.

Having this information passed as its own custom header also allows any existing use of User-Agent to continue without interference.

designatednerd added 4 commits December 16, 2019 17:05

make protocol for headers get-only

98a0053

Add convenience method to add client headers to a request for any tra…

16134f7

…nsport

update tests to use instance instead of static methods since I change…

22ba28b

…d that

add note about client name and version to the main send method.

3105441

designatednerd added 3 commits December 16, 2019 17:25

use add headers for web socket

e0e25b1

use getter only for split client

be19058

update split client tests

befa6b5

designatednerd added this to the 0.21.0 milestone Dec 16, 2019

gsabran reviewed Dec 16, 2019

View reviewed changes

Sources/Apollo/NetworkTransport.swift Outdated Show resolved Hide resolved

designatednerd added 2 commits December 16, 2019 18:02

clarify that these are headers for apollo specifically

1f07328

Clean up note on usage to try and make it clearer.

72fe6aa

Add default implementation which actually uses default values

24e3c64

cysp reviewed Dec 19, 2019

View reviewed changes

revert change from static to instance variable for header field names

08a5065

cysp reviewed Dec 19, 2019

View reviewed changes

cysp approved these changes Dec 19, 2019

View reviewed changes

Update Sources/ApolloWebSocket/SplitNetworkTransport.swift

08fe052

Co-Authored-By: Scott Talbot <stalbot@scentregroup.com>

designatednerd merged commit 24abcb0 into master Dec 19, 2019

designatednerd deleted the update/gettable-only branch January 15, 2020 13:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `clientName` and `clientVersion` get-only protocol requirements #954

Make `clientName` and `clientVersion` get-only protocol requirements #954

designatednerd commented Dec 16, 2019

designatednerd commented Dec 16, 2019

gsabran commented Dec 16, 2019 •

edited

Loading

designatednerd commented Dec 16, 2019

designatednerd commented Dec 17, 2019

gsabran commented Dec 17, 2019

designatednerd commented Dec 17, 2019

cysp commented Dec 18, 2019

designatednerd commented Dec 18, 2019

cysp Dec 19, 2019

designatednerd Dec 19, 2019

cysp Dec 19, 2019

cysp Dec 19, 2019

bwhiteley commented Dec 19, 2019

bwhiteley commented Dec 19, 2019

bwhiteley commented Dec 19, 2019

bwhiteley commented Dec 19, 2019

designatednerd commented Dec 19, 2019

Make clientName and clientVersion get-only protocol requirements #954

Make clientName and clientVersion get-only protocol requirements #954

Conversation

designatednerd commented Dec 16, 2019

designatednerd commented Dec 16, 2019

gsabran commented Dec 16, 2019 • edited Loading

designatednerd commented Dec 16, 2019

designatednerd commented Dec 17, 2019

gsabran commented Dec 17, 2019

designatednerd commented Dec 17, 2019

cysp commented Dec 18, 2019

designatednerd commented Dec 18, 2019

cysp Dec 19, 2019

Choose a reason for hiding this comment

designatednerd Dec 19, 2019

Choose a reason for hiding this comment

cysp Dec 19, 2019

Choose a reason for hiding this comment

cysp Dec 19, 2019

Choose a reason for hiding this comment

bwhiteley commented Dec 19, 2019

bwhiteley commented Dec 19, 2019

bwhiteley commented Dec 19, 2019

bwhiteley commented Dec 19, 2019

designatednerd commented Dec 19, 2019

Make `clientName` and `clientVersion` get-only protocol requirements #954

Make `clientName` and `clientVersion` get-only protocol requirements #954

gsabran commented Dec 16, 2019 •

edited

Loading