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

Reply via email to