This is from Andrew's wiki comment. Sorry to paste it back to the list, but I'm having some difficulty commenting there:
> > 1. Setters with no getters > Philosophically I don't agree that you need to make all properties > read/write. I see no particular reason to make these properties readable > since they will never change once they are set, or in the case of the > password should actually not be accessible once set (because the > implementation *should* be scrubbing the bytes from memory after use). > In my view if the application needs the value again it already has it. > In the case of the read-only property authenticated user I definitely > think that needs to be read only. > Having said that, I don't feel that strongly about getters for the > client username and hostname. > > There's actually an important point here worth noting. With reactive use, I don't think it's true, pragmatically speaking, that the application has the value again when it needs it. When your primary means of operating on a connection is through handling events, the only state you have easy access to is what is provided by those events. Taking your suggestion, if I wanted to do something simple like log a debug statement from an event handler and include the hostname and/or username of the connection in that info, my only recourse would be to malloc some sort of ancillary struct and attach it to the connection and fill it in with the very same hostname that the connection is holding internally, or alternatively access some sort of global state that holds a map from the connection back to that same information. If your point is that this is possible then of course that's true, but it doesn't seem at all reasonable. > > 1. inconsistency with existing use of "remote" in API > I take your point - I'm happy to remove "remote" from the API name - > would "connected" be all right? pn_transport_set_hostname() just > doesn't seem specific enough to me - it might just as well be telling the > transport what *our* hostname is. > 2. Redundancy of pn_connection_set_hostname() and > pn_transport_set_<something>_hostname() > Yes these are definitely redundant, and I would need to deprecate the > connection version and set it from the transport when binding them together > - good catch. > The transport version must be primary, as (in principle at least, if > not in the current implementation) you don't need the connection object > until you have authenticated the transport and authentication and > encryption may to know need the hostname you are connecting to. I think it > would have to be an error to rebind (on reconnect) a connection with a > different hostname than the transport hostname. > > This isn't consistent with how the connection and transport are actually used. With the reactive API, the user creates the connection endpoint first and configures it with all the relevant details that it cares about. The transport isn't created at all until the client actually opens the connection (which could be somewhere completely different from where it configures the connection). It's also important to note that the user doesn't actually create the transport at all. The default handlers do this when the PN_CONNECTION_LOCAL_OPEN event occurs. The users don't even need to be aware that a transport object exists at all if they don't care to customize it. This is a nice property and would be difficult to maintain if you start pushing connection level configuration like hostname into the transport. I think if you flip things around it actually makes more sense. As a server you are going to have a transport prior to having a connection, and in this case you want to access the hostname-that-your-remote-peer desires for vhost purposes, but it makes no sense to actually set it. As a client, a transport is pretty much useless until it is bound to a connection, as you can't safely do much more than send the protocol header, so the natural sequence is to create the connection first and set the hostname you want to connect to, and not worry about the Transport. > > 1. Having a separate set_user/set_password > That would make sense. However from this conversation I'm wondering > actually if we should more carefully distinguish the client and server > ends. And so have a client only API to set user/password and a server only > API to extract the authenticated username. > > So in conclusion how about: > > - Changing pn_transport_set_remote_hostname() → > pn_transport_set_connect_hostname() (connect/connected/connection?) > - Adding pn_transport_get_connect_hostname() > (connect/connected/connection?) > - Deprecating pn_connection_set/get_hostname() in favour of > pn_transport_set/get_connect_hostname() > Actually changing the pn_transport_bind() code would be required too. > - Removing pn_transport_set_user_password() and pn_transport_get_user() > - Replacing them with pn_transport_set_client_user(), > pn_set_client_password(), pn_transport_get_client_user() and > pn_transport_get_authenticated_user(). > > It might make sense to follow a similar pattern as the hostname here, i.e. in the client scenario you configure the connection with pn_connection_get/set_user() prior to the existence of the transport, and in the server scenario the transport simply provides pn_transport_remote_user(...). > You'll note I've added in the getters that I don't think are necessary, > but that you do (hostname and client_user), but I've maintained the > password property as write only and the authenticated user as read only. > I wasn't suggesting that you need to provide an accessor for the password, I was suggesting that if you don't provide one, you shouldn't call the "setter" pn_transport_set_password(...) since that will violate expectations by not having a corresponding pn_transport_get_password(...). --Rafael
