Add inProcessExecution that propagates the conjure exceptions #1516

crogoz · 2024-02-29T16:08:28Z

Before this PR

Conjure exceptions were not propagated to gradle-conjure. We were only getting the exitCode.

we were converting any exception into an exitCode: https://github.com/palantir/conjure/blob/f2cf6baa77d78b0aa2e6f3c1343542904756c1ee/conjure/src/main/java/com/palantir/conjure/cli/ConjureCli.java#L58C45-L58C46
in gradle-conjure we were only getting the exitCode: https://github.com/palantir/gradle-conjure/blob/003f0e5b4c2d928a49183c762ec4f09ed14573d2/gradle-conjure/src/main/java/com/palantir/gradle/conjure/ConjureRunnerResource.java#L198

before the exception looked like:

* What went wrong:
Execution failed for task ':types-api:rawIr'.
> Failed to generate conjure IR. The command 'conjure' with args [compile, /Volumes/git/service/types-api/build/conjure, /Volumes/git/service/types-api/build/conjure-ir/rawIr.conjure.json, --extensions, {"recommended-product-dependencies":[]}, --requireSafety] failed with exit code -1. Output above.

After this PR

Introduces a new inProcessExecution method that will re-throw any exception

* What went wrong:
Execution failed for task ':types-api:rawIr'.
> com.palantir.conjure.exceptions.ConjureIllegalStateException: Unknown LocalReferenceType: TypeName{name=Name1}
  
  Failed to generate conjure IR. The command '[compile, /Volumes/git/service/types-api/build/conjure, /Volumes/git/service/types-api/build/conjure-ir/rawIr.conjure.json, --extensions, {"recommended-product-dependencies":[]}, --requireSafety]' failed.

==COMMIT_MSG==
Add inProcessExecution that propagates the conjure exceptions
==COMMIT_MSG==

Possible downsides?

changelog-app · 2024-02-29T16:08:32Z

Generate changelog in `changelog/@unreleased`

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

Description

Add inProcessExecution that propagates the conjure exceptions

Check the box to generate changelog(s)

Generate changelog entry

CRogers · 2024-03-04T13:48:28Z

conjure/src/main/java/com/palantir/conjure/cli/ConjureCli.java

            if (ex == null) {
+                thrownException = Optional.ofNullable(ex);


This one seems suspect - ex is null here, so we probably don't need this line.

…cr/capture-stderr

CRogers · 2024-03-04T13:50:49Z

conjure/src/main/java/com/palantir/conjure/cli/ConjureCli.java

+                            ? handleJsonMappingException((JsonMappingException) exception)
+                            : exception.getMessage())
+                    .collect(Collectors.joining("\n"));
+            commandLine.getErr().println(message);


What happens to this when called in process? Does it still print out to stderr?

Looks like there is a setErr we can use on CommandLine to change it.

Yes, it does still log to stderr. the getErr() is the default [standardError](https://github.com/remkop/picocli/blob/main/src/main/java/picocli/CommandLine.java#L1311C16-L1311C19).

CRogers · 2024-03-04T13:57:31Z

conjure/src/main/java/com/palantir/conjure/cli/ConjureCli.java

    @VisibleForTesting
    static CommandLine prepareCommand() {
        return new CommandLine(new ConjureCli()).setExecutionExceptionHandler(new ExceptionHandler());
    }

    public static final class ExceptionHandler implements IExecutionExceptionHandler {
+
+        private Optional<Exception> thrownException = Optional.empty();


I actually think we can write this a little more concisely/safely for future changes. We can keep the existing IExecutionExceptionHandler as is then write a wrapper IExecutionExceptionHandler that wraps the original one but catches the exception and saves it. Also sets the err and if that is written to, we make an exception from that.

Perhaps with a TeeOutputStream to the original err if we want to preserve the current behaviour (maybe we don't need to though?)

added a separate class that does the exception wrapping here: https://github.com/palantir/conjure/pull/1516/files#diff-cc1e8eaeed51e87c5b283c659588f4c2f976ceafdc8ad587e89fc92460fdccedR33.

Invalidated by push of e840abf

svc-autorelease · 2024-03-11T15:06:10Z

Released 4.40.0

crogoz added 5 commits February 26, 2024 16:42

exception store

ac28d26

conjure

eae0e67

do not commit

9fb6c5c

add tests

c1d8d62

add tests

8e37ee8

crogoz requested a review from CRogers February 29, 2024 16:08

Add generated changelog entries

79104a9

crogoz mentioned this pull request Feb 29, 2024

Propagate exceptions from conjure palantir/gradle-conjure#1369

Draft

CRogers reviewed Mar 4, 2024

View reviewed changes

crogoz added 2 commits March 4, 2024 14:50

rm line

c0afd23

Merge branch 'cr/capture-stderr' of github.com:palantir/conjure into …

46815da

…cr/capture-stderr

CRogers reviewed Mar 4, 2024

View reviewed changes

crogoz added 2 commits March 4, 2024 17:07

add throwable handler

aa6b779

address comments

e840abf

CRogers previously approved these changes Mar 4, 2024

View reviewed changes

crogoz force-pushed the cr/capture-stderr branch from 2057ae9 to e840abf Compare March 4, 2024 16:16

crogoz requested a review from CRogers March 4, 2024 16:17

CRogers approved these changes Mar 11, 2024

View reviewed changes

CRogers added merge when ready autorelease labels Mar 11, 2024

bulldozer-bot bot merged commit a55ea92 into master Mar 11, 2024
5 checks passed

bulldozer-bot bot deleted the cr/capture-stderr branch March 11, 2024 15:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add inProcessExecution that propagates the conjure exceptions #1516

Add inProcessExecution that propagates the conjure exceptions #1516

crogoz commented Feb 29, 2024

changelog-app bot commented Feb 29, 2024 •

edited by crogoz

CRogers Mar 4, 2024

CRogers Mar 4, 2024

CRogers Mar 4, 2024

crogoz Mar 4, 2024 •

edited

CRogers Mar 4, 2024

CRogers Mar 4, 2024

crogoz Mar 4, 2024

CRogers Mar 4, 2024

svc-autorelease commented Mar 11, 2024

Add inProcessExecution that propagates the conjure exceptions #1516

Add inProcessExecution that propagates the conjure exceptions #1516

Conversation

crogoz commented Feb 29, 2024

Before this PR

After this PR

Possible downsides?

changelog-app bot commented Feb 29, 2024 • edited by crogoz

Generate changelog in changelog/@unreleased

CRogers Mar 4, 2024

Choose a reason for hiding this comment

CRogers Mar 4, 2024

Choose a reason for hiding this comment

CRogers Mar 4, 2024

Choose a reason for hiding this comment

crogoz Mar 4, 2024 • edited

Choose a reason for hiding this comment

CRogers Mar 4, 2024

Choose a reason for hiding this comment

CRogers Mar 4, 2024

Choose a reason for hiding this comment

crogoz Mar 4, 2024

Choose a reason for hiding this comment

CRogers Mar 4, 2024

Choose a reason for hiding this comment

svc-autorelease commented Mar 11, 2024

changelog-app bot commented Feb 29, 2024 •

edited by crogoz

Generate changelog in `changelog/@unreleased`

crogoz Mar 4, 2024 •

edited