Make exceptions a bit more informative #1255

yuri-sergiichuk · 2020-03-25T16:48:54Z

In the PR I have added some more informative context to exceptions thrown by the framework.

The initial trigger for the change was an exception thrown by the migration process with as little info as:

java.lang.IllegalArgumentException: An entity with ID `order_id {
  date {
    year: 2020
    month: JANUARY
    day: 15
  }
  number: 1
}
` is not found in the repository.
	at io.spine.util.Exceptions.newIllegalArgumentException(Exceptions.java:164)
	at io.spine.server.entity.RecordBasedRepository.lambda$findOrThrow$1(RecordBasedRepository.java:428)

codecov · 2020-03-25T16:57:57Z

Codecov Report

Merging #1255 into master will increase coverage by 0.00%.
The diff coverage is 80.00%.

@@            Coverage Diff            @@
##             master    #1255   +/-   ##
=========================================
  Coverage     91.09%   91.10%           
  Complexity     4641     4641           
=========================================
  Files           596      596           
  Lines         14678    14683    +5     
  Branches        831      831           
=========================================
+ Hits          13371    13377    +6     
  Misses         1046     1046           
+ Partials        261      260    -1

dmdashenkov · 2020-03-25T16:52:43Z

.idea/inspectionProfiles/Project_Default.xml

@@ -74,6 +74,7 @@
    <inspection_tool class="BigDecimalEquals" enabled="true" level="WARNING" enabled_by_default="true" />
    <inspection_tool class="BreakStatement" enabled="true" level="WARNING" enabled_by_default="true" />
    <inspection_tool class="BreakStatementWithLabel" enabled="true" level="WARNING" enabled_by_default="true" />
+    <inspection_tool class="BusyWait" enabled="true" level="WARNING" enabled_by_default="true" />


Please synchronize this file with the config module. It might be that your local settings are infiltrating the repository.

I actually got these changes from the config/pull. Right now they are identical.

dmdashenkov · 2020-03-25T16:54:12Z

server/src/main/java/io/spine/server/aggregate/AggregateRepository.java

@@ -431,7 +432,8 @@ final void importEvent(EventEnvelope event) {
            I id = ids.stream()
                      .findFirst()
                      .orElseThrow(() -> newIllegalStateException(
-                              "Unable to route import event `%s`.", messageType)
+                              "Unable to route import event `%s`. Event: %s",
+                              messageType, shortDebugString(message))


Logging an event body is a security concern. Please only log the meta-data, such as event ID and timestamp.

dmdashenkov · 2020-03-25T16:56:12Z

server/src/main/java/io/spine/server/aggregate/AggregateRepository.java

-            throw newIllegalStateException("Aggregate %s (ID: %s) cannot be loaded.%n",
-                                           aggregateClass().value().getName(),
-                                           result.idAsString());
+            throw newIllegalStateException(


Please revert the formatting. The policy is not to split the method and the first param if possible, i.e. if the char count is not too big.

dmdashenkov · 2020-03-25T16:56:46Z

server/src/main/java/io/spine/server/aggregate/AggregateRepository.java

-                                           aggregateClass().value().getName(),
-                                           result.idAsString());
+            throw newIllegalStateException(
+                    "Aggregate %s (ID: %s) cannot be loaded.%n",


Suggested change

"Aggregate %s (ID: %s) cannot be loaded.%n",

"Aggregate `%s` (ID: %s) cannot be loaded.%n",

dmdashenkov · 2020-03-25T16:58:25Z

server/src/main/java/io/spine/server/commandbus/CommandAckMonitor.java

@@ -112,7 +112,11 @@ private static EventMessage systemEventFor(Status status, CommandId commandId) {
                                     .build();
            case REJECTION:
            default:
-                throw newIllegalArgumentException("Invalid status %s.", status.getStatusCase());
+                throw newIllegalArgumentException(


Has this error popped up in your log? If so, this is a bug or a compatibility issue worth paying attention to. Let's discuss if needed.

Nope, not really. I looked over the other messages that could have a bit more useful info.

dmdashenkov · 2020-03-25T16:59:34Z

server/src/main/java/io/spine/server/entity/RecordBasedRepository.java

@@ -426,7 +428,7 @@ protected E findOrCreate(I id) {

    private E findOrThrow(I id) {
        return find(id).orElseThrow(() -> newIllegalArgumentException(
-                "An entity with ID `%s` is not found in the repository.", id
+                "An entity `%s` with ID `%s` is not found in the repository.", entityClass(), id


Please make sure the word class is not added to the message before the class name.

dmdashenkov

@yuri-sergiichuk, LGTM with a single comment to address.

dmdashenkov · 2020-03-25T17:45:56Z

server/src/main/java/io/spine/server/aggregate/AggregateRepository.java

@@ -431,7 +431,8 @@ final void importEvent(EventEnvelope event) {
            I id = ids.stream()
                      .findFirst()
                      .orElseThrow(() -> newIllegalStateException(
-                              "Unable to route import event `%s`.", messageType)
+                              "Unable to route import event `%s`. Event: %s",


There's still a placeholder left.

yuri-sergiichuk added 6 commits March 25, 2020 17:53

Add entity class name to the thrown exception

4e714cb

Add the command ID value to the exception message.

d1e1ab2

Add more debug information to the exception

e933122

Add the message itself to provide more context in the exception

03c34e9

Pull latest config module

7421f03

Bump version

4ffe82d

yuri-sergiichuk requested a review from dmdashenkov March 25, 2020 16:48

yuri-sergiichuk self-assigned this Mar 25, 2020

dmdashenkov suggested changes Mar 25, 2020

View reviewed changes

yuri-sergiichuk added 3 commits March 25, 2020 19:35

Fix the review comments

8d370a6

Use the class canonical name

34880a1

Add missing parameter

565875b

yuri-sergiichuk requested a review from dmdashenkov March 25, 2020 17:41

dmdashenkov approved these changes Mar 25, 2020

View reviewed changes

Remove the obsolete placeholder.

375fd74

yuri-sergiichuk merged commit 6df1389 into master Mar 25, 2020

yuri-sergiichuk deleted the make-exceptions-informative branch March 25, 2020 18:06

dmitrykuzmin mentioned this pull request Sep 8, 2020

Release 1.6.0 #1302

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make exceptions a bit more informative #1255

Make exceptions a bit more informative #1255

yuri-sergiichuk commented Mar 25, 2020

codecov bot commented Mar 25, 2020 •

edited

Loading

dmdashenkov Mar 25, 2020

yuri-sergiichuk Mar 25, 2020 •

edited

Loading

dmdashenkov Mar 25, 2020

dmdashenkov Mar 25, 2020

dmdashenkov Mar 25, 2020

dmdashenkov Mar 25, 2020

yuri-sergiichuk Mar 25, 2020

dmdashenkov Mar 25, 2020

dmdashenkov left a comment

dmdashenkov Mar 25, 2020

	"Aggregate %s (ID: %s) cannot be loaded.%n",
	"Aggregate `%s` (ID: %s) cannot be loaded.%n",

Make exceptions a bit more informative #1255

Make exceptions a bit more informative #1255

Conversation

yuri-sergiichuk commented Mar 25, 2020

codecov bot commented Mar 25, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

yuri-sergiichuk Mar 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmdashenkov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 25, 2020 •

edited

Loading

yuri-sergiichuk Mar 25, 2020 •

edited

Loading