SafeLoggable exception for invalid UUID values

[ash211]

Before this PR

Parsing a too-long UUID string fails with an IllegalArgumentException which does not include the invalid value:

java.lang.IllegalArgumentException: UUID string too large
        at java.base/java.util.UUID.fromString1(UUID.java:264)
        at java.base/java.util.UUID.fromString(UUID.java:258)
        at com.palantir.example.BlockInternalId.valueOf(BlockInternalId.java:45)

openjdk/jdk source

See https://pl.ntr/2l6 for an internal example stacktrace.

This makes it difficult to debug, as there is no log of the value, even at unsafe level, to assist with debugging.

After this PR

==COMMIT_MSG== SafeLoggable exception for invalid UUID values ==COMMIT_MSG==

Possible downsides?

Potentially there is some performance impact from this change for parsing invalid UUID aliases, or even valid UUIDs. I think it's likely slight -- just wrapped in a try block for the valid case, and creating a wrapping exception for the invalid case.

[changelog-app[bot]]

Generate changelog in changelog-dir>`changelog/@unreleased`</changelog-dir

What do the change types mean?

- `feature`: A new feature of the service. - `improvement`: An incremental improvement in the functionality or operation of the service. - `fix`: Remedies the incorrect behaviour of a component of the service in a backwards-compatible way. - `break`: Has the potential to break consumers of this service's API, inclusive of both Palantir services and external consumers of the service's API (e.g. customer-written software or integrations). - `deprecation`: Advertises the intention to remove service functionality without any change to the operation of the service itself. - `manualTask`: Requires the possibility of manual intervention (running a script, eyeballing configuration, performing database surgery, ...) at the time of upgrade for it to succeed. - `migration`: A fully automatic upgrade migration task with no engineer input required. _Note: only one type should be chosen._

How are new versions calculated?

- ❗The `break` and `manual task` changelog types will result in a major release! - 🐛 The `fix` changelog type will result in a minor release in most cases, and a patch release version for patch branches. This behaviour is configurable in autorelease. - ✨ All others will result in a minor version release.

Type

- [ ] Feature - [x] Improvement - [ ] Fix - [ ] Break - [ ] Deprecation - [ ] Manual task - [ ] Migration

Description

SafeLoggable exception for invalid UUID values **Check the box to generate changelog(s)** - [x] Generate changelog entry

[carterkozak]

@ash211 I'm not sure this should be necessary/worth the additional generated exception handling.

In the worst case, the stack trace points to the point in the jdk uuid parser which throws the error, making it clear what the cause was. Not the best devx, but workable. However, unless a service is misconfigured (missing the constant-strings javaagent), the exception message constant will be considered safe by our infrastructure, so it's not clear to me that making this change would buy us anything.

[carterkozak]

Ah, I misread the problem you're solving -- not that the exception isn't safeloggable, but that we don't have a way to understand the bad input. I think we can also leverage the conjure log-safety context to log bad inputs as SafeArg instead of UnsafeArg. However, we must also be careful not to include anything tagged do-not-log as UnsafeArg.

[ash211]

Sorry, I should've made the PR title/description more clear. You're right that I'm trying to solve "what was the invalid input?" and not "the thrown exception doesn't inherit from SafeLoggable"

Being able to promote UnsafeArgs to SafeArgs where possible would be a nice extension! In the particular case that prompted this the user had access to unsafe logs (on an IL), but I acknowledge that's not the common case.

In the meantime, I was thinking it's safe to log at UnsafeArg level, since that's what ResourceIdentifier.of(String) does, and it can receive any arbitrary String inputs.

Maybe we can use the conjure log-safety context (not entirely sure what that is) to improve both UuidAliasExample.valueOf and ResourceIdentifier.valueOf. Do you have any pointers on how to do this? Happy to FLUP with that if you can give a bit of guidance.

[carterkozak]

@ash211 The computed/combined log-safety is evaluated here: https://github.com/palantir/conjure-java/blob/2ad5a433bf2355461253a0888ce7eccc27ec7cd3/conjure-java-core/src/main/java/com/palantir/conjure/java/types/AliasGenerator.java#L76C29-L76C43

It's used to set the type-level safety annotations on the class.

In the meantime, I was thinking it's safe to log at UnsafeArg level, since that's what ResourceIdentifier.of(String) does, and it can receive any arbitrary String inputs.

I think that may be a bit questionable, though probably fine in practice because resource-id has a more specific intent than uuid.

If we go down this path, I think we should attempt to solve both Alias.valueOf(String) (for all aliases which produce such a method, not exclusively UUID), and the PlainSerDe deserializers for header/query/path params. I'm not sure we can do anything quite as helpful for jackson deserialization, but it's a start.