make defacto public fields actually public

[sunshowers]

These fields are effectively public through the Deserialize impl -- make them public to align with this fact.

I searched for Deserialize (case-sensitive) in dropshot/src and these were the main ones I could find. There's also DeserializedConfigDropshot but that isn't pub.

Closes #1525.

[david-crespo]

seems fine!

[ahl]

Does it follow that a field that is deserialized should be pub? I'm not sure that's always the case. For pagination in particular, I could argue that shielding users from this internal detail might be a feature?

[ahl]

what about this? I'm not sure I recall the rationale here; perhaps to preserve our ability to change things in the future?

[sunshowers]

It's not possible to preserve the ability without a breaking change, since this is directly exposed in the HTTP endpoint.

[davepacheco]

Mentioned elsewhere but so this is not lost: the reason this is not pub is because it is almost certainly the wrong thing for consumers to read it so it would be a footgun to expose it to them. They should be using page_limit(). (This field is the limit the user (untrusted) may have provided. page_limit() looks at this, but validates it (if specified) and provides a default (if not).)

[ahl]

I'm not sure I follow--this type isn't public, right?

[sunshowers]

ah yeah this type isn't public, my bad

[sunshowers]

Does it follow that a field that is deserialized should be pub?

If there's no validation as part of deserializing, yes. Not having it be pub (or similar, e.g. a constructor) is misleading.

I'm not sure that's always the case. For pagination in particular, I could argue that shielding users from this internal detail might be a feature?

But it's not an internal detail, since it is exposed via the deserializer, in the JSON schema, etc. Making it private gives the misleading impression that it is an internal detail, which is exactly the problem here.

[ahl]

I feel somewhere in the realm of ambivalence and indifference: I don't think this addresses any particular need which makes me inclined to leave it as-is (in particular in light of that comment). However I don't feel at all strongly so... 🤷

[davepacheco]

Does it follow that a field that is deserialized should be pub?

If there's no validation as part of deserializing, yes. Not having it be pub (or similar, e.g. a constructor) is misleading.

I'm not sure that's always the case. For pagination in particular, I could argue that shielding users from this internal detail might be a feature?

But it's not an internal detail, since it is exposed via the deserializer, in the JSON schema, etc. Making it private gives the misleading impression that it is an internal detail, which is exactly the problem here.

I don't follow this logic. What's wrong or misleading about having a pub type with private fields that can be deserialized, but only in an opaque way (namely, you can only deserialized with the implementation itself serialized)?

I think it's rather misleading to make these pub as though the consumer could reasonably expect to be able to set or modify these fields on their own.

In the specific case of limit: we're not trying to hide the serialization, but it's almost certainly wrong for the caller to access this field directly instead of using the page_limit() method. Making the field pub feels like giving the consumer a footgun.

[sunshowers]

Oh sorry, thought I linked the specific use case for this over here -- it's oxidecomputer/omicron#9723 where I had to go through serde_json::Value to create a new instance. (Looks like I linked it in the attached issue, but not here, sorry!)

I don't follow this logic. What's wrong or misleading about having a pub type with private fields that can be deserialized, but only in an opaque way (namely, you can only deserialized with the implementation itself serialized)?

That does seem like a special case where it would be okay, but personally that's not how I'd write it. I'd not expose Serialize and Deserialize impls in that case since it exposes genericity over serialization format which wouldn't make sense. Then I'd have a layer of indirection: private type which does the serialization/deserialization, public domain type, and a function to convert to/from raw bytes.

I do think this is out the window the moment a JSON schema is exposed, since that is part of the public API.

[davepacheco]

Ah, I understand better and replied over on the Omicron issue.

[sunshowers]

Closing in favor of #1528.