Make encryption opt-in on NpgsqlSlimDataSourceBuilder#4976
Conversation
roji
force-pushed
the
EncryptionBeGone
branch
2 times, most recently
from
db2cacb to
e0774a5
Compare
vonzshik
left a comment
vonzshik
left a comment
There was a problem hiding this comment.
Looks great, one small nit
| /// Enables to possibility to use TLS/SSl encryption for connections to PostgreSQL. This does not guarantee that encryption will | ||
| /// actually be used; see <see href="https://www.npgsql.org/doc/security.html"/> for more details. | ||
| /// </summary> | ||
| public NpgsqlSlimDataSourceBuilder EnableEncryption() |
There was a problem hiding this comment.
We could just call it Add* instead of enable
There was a problem hiding this comment.
I looked over it the naming again... Most of the other APIs on the builder are called Use*; but they actually mean that the feature will be used (e.g. UseClientCertificate, UseLoggerFactory). There's EnableParameterLogging, which is about turning a feature on or off (UseParameterLogging makes no sense), and also aligns with EF (in case that matters.
I think EnableEncryption and EnableRanges is consistent - it's just turning a feature on... SystemTextJson is annoying, since it's both turning on the possibility to use JSON (if using the slim builder) and also configuring it with serializer options etc. (which is why it's also exposed on the non-slim builder).
Long story short... Add also seems fine; I admit I slightly prefer Enable since it's very clearly about turning something on/off; but it's also consistent with the existing EnableParameterLogging, which is also a point in its favor...
I'll go ahead and merge this for now, but am perfectly ok rediscussing the naming and changing.
3 participants
Closes #4966