accept list arg as parameter in preconditions

barisoyoruk commented 3 weeks ago

Before this PR

We cannot pass List<Arg<?>> to Preconditions methods unlike SafeLogger methods which create duality between those two classes.

After this PR

We can also pass List<Arg<?>> to Preconditions methods like SafeLogger methods.

SafeLogger methods only support List<Arg<?>> Preconditions methods only support Arg<?>...

In our service, we have an utility function that returns List<Arg<?>>. It makes sense for it to work with both Preconditions and SafeLogger without requiring converting List to Arg<?>[] each time.

==COMMIT_MSG== Preconditions' methods now accept List<Arg<?>> as a parameter. ==COMMIT_MSG==

Possible downsides?

Should we also add Arg<?>... definition to the SafeLogger? I'd except at least one common parameter type between Exceptions, SafeLogger, and Preconditions
Should we also duplicate the work inside Exceptions?

changelog-app[bot] commented 3 weeks ago

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

Preconditions' methods now accept List> as a parameter.

**Check the box to generate changelog(s)** - [x] Generate changelog entry

pkoenig10 commented 3 weeks ago

Should we also add Arg<?>... definition to the SafeLogger? I'd except at least one common parameter type between Exceptions, SafeLogger, and Preconditions

SafeLogger already has explicit methods for arg arity up to 10. I think the design of SafeLogger is objectively better because it avoids allocating arrays in the vast majority of cases.

Should we also duplicate the work inside Exceptions?

Probably, because it will allow us to avoid an array copy here.

carterkozak commented 3 weeks ago

Another consideration is that Preconditions calls with several Arg<?>s are usually better off being replaced thusly:

- Preconditions.checkState(condition, "message", SafeArg.of("key", value));
+ if (!condition) {
+     // Arg instances are only allocated when the condition is false, never in the successful path
+     throw new SafeIllegalStateException("message", SafeArg.of("key", value));
+ }

Secondly, matching behavior of the SafeLogger isn't particularly compelling. It's an antipattern to both throw and log because it splits context between at least two log lines for the same failure -- we expect throwables to be logged when they're caught. There are some counterexamples, to be sure, but I'd really like to understand a bit more context behind this request.

barisoyoruk commented 3 weeks ago

Another consideration is that Preconditions calls with several Arg<?>s are usually better off being replaced thusly:
- Preconditions.checkState(condition, "message", SafeArg.of("key", value));
+ if (!condition) {
+     // Arg instances are only allocated when the condition is false, never in the successful path
+     throw new SafeIllegalStateException("message", SafeArg.of("key", value));
+ }
Secondly, matching behavior of the SafeLogger isn't particularly compelling. It's an antipattern to both throw and log because it splits context between at least two log lines for the same failure -- we expect throwables to be logged when they're caught. There are some counterexamples, to be sure, but I'd really like to understand a bit more context behind this request.

So basically I am working on a new logging system in alta-client-java that lets clients to decide whether their keys are safe or not. For that reason I have this class: https://github.palantir.build/foundry/alta-client-java/pull/5077/files#diff-309cd77788c65c6d5993784651d6e07469fc4eb97ee1e724eb30a41f6b5c3542

Whenever someone wants to log something via SafeLogger or check via Preconditions, they will call the methods inside this class. Currently methods return List<Arg<?>> which works nicely with SafeLogger. However, when we want to used them with Preconditions, we need to convert them to Arg<?>[] each time which is annoying.

I think for this kind of utility methods, we'd better have a common parameter in methods that works with Args, either via List<Arg<?>> or Arg<?>...

Does it make sense? @carterkozak

palantir / safe-logging