Add New...f constructors for all service errors #221

alexshtin · 2025-05-12T21:56:38Z

What changed?
Add New...f constructors for all service errors.

Why?
We have similar helpers in different places but they all belong to this repo.

How did you test it?
Didn't test.

dnr · 2025-05-14T01:30:05Z

serviceerror/already_exists.go

+// NewAlreadyExistf returns new AlreadyExists error with formatted message.
+func NewAlreadyExistf(format string, args ...interface{}) error {


Suggested change

// NewAlreadyExistf returns new AlreadyExists error with formatted message.

func NewAlreadyExistf(format string, args ...interface{}) error {

// NewAlreadyExistsf returns new AlreadyExists error with formatted message.

func NewAlreadyExistsf(format string, args ...interface{}) error {

and ideally the non-f one should be renamed too

dnr · 2025-05-14T01:30:56Z

serviceerror/multi_op.go

+// NewMultiOperationExecutionf returns a new MultiOperationExecution error with formatted message.
+func NewMultiOperationExecutionf(format string, errs []error, args ...interface{}) error {


weird to have format and args separated, how about switching the order?

dnr · 2025-05-14T01:31:40Z

serviceerror/namespace_invalid_state.go

@@ -40,6 +40,16 @@ func NewNamespaceInvalidState(namespace string, state enumspb.NamespaceState, al
 	}
 }

+// NewNamespaceInvalidStatef returns new NamespaceInvalidState error with formatted message.
+func NewNamespaceInvalidStatef(namespace string, state enumspb.NamespaceState, allowedStates []enumspb.NamespaceState, format string, args ...interface{}) error {


do we really need this one? the main constructor does a good job with messages...

dnr · 2025-05-14T01:32:08Z

serviceerror/namespace_not_active.go

@@ -35,6 +35,16 @@ func NewNamespaceNotActive(namespace, currentCluster, activeCluster string) erro
 	}
 }

+// NewNamespaceNotActivef returns new NamespaceNotActive error with formatted message.
+func NewNamespaceNotActivef(namespace, currentCluster, activeCluster, format string, args ...interface{}) error {


same here...

dnr · 2025-05-14T01:32:51Z

serviceerror/namespace_unavailable.go

@@ -26,6 +26,14 @@ func NewNamespaceUnavailable(namespace string) error {
 	}
 }

+// NewNamespaceUnavailablef returns new NamespaceUnavailable error with formatted message.
+func NewNamespaceUnavailablef(namespace, format string, args ...interface{}) error {


this probably shouldn't exist, NamespaceUnavailable has no Message field

dnr · 2025-05-14T01:33:12Z

serviceerror/newer_build_exists.go

@@ -27,6 +27,14 @@ func NewNewerBuildExists(defaultBuildID string) error {
 	}
 }

+// NewNewerBuildExistsf returns new NewerBuildExists error with formatted message.
+func NewNewerBuildExistsf(defaultBuildID, format string, args ...interface{}) error {


maybe not needed?

dnr · 2025-05-14T01:34:22Z

serviceerror/resource_exhausted.go

@@ -26,6 +27,14 @@ func NewResourceExhausted(cause enumspb.ResourceExhaustedCause, message string)
 	}
 }

+// NewResourceExhaustedf returns new ResourceExhausted error with formatted message.
+func NewResourceExhaustedf(cause enumspb.ResourceExhaustedCause, format string, args ...interface{}) error {


should we have constructors that take Scope also? we always set it everywhere now, right? I feel like we'll never get to use this one because we need a Scope

dnr · 2025-05-14T01:34:40Z

serviceerror/server_version_not_supported.go

@@ -28,6 +28,15 @@ func NewServerVersionNotSupported(serverVersion, supportedVersions string) error
 	}
 }

+// NewServerVersionNotSupportedf returns new ServerVersionNotSupported error with formatted message.
+func NewServerVersionNotSupportedf(serverVersion, supportedVersions, format string, args ...interface{}) error {


feel like the existing message should be fine everywhere?

dnr · 2025-05-14T01:35:28Z

serviceerror/system_workflow.go

@@ -27,6 +27,14 @@ func NewSystemWorkflow(workflowExecution *common.WorkflowExecution, workflowErro
 	}
 }

+// NewSystemWorkflowf returns new SystemWorkflow error with formatted workflow error.
+func NewSystemWorkflowf(workflowExecution *common.WorkflowExecution, format string, args ...interface{}) error {


I can't imagine ever using this, it should always be constructed from an error

cretz · 2025-05-14T13:10:27Z

serviceerror/already_exists.go

@@ -13,13 +15,20 @@ type (
 	}
 )

-// NewAlreadyExist returns new AlreadyExists error.
-func NewAlreadyExist(message string) error {


We can't make backwards-incompatible changes to this package IMO (even if users shouldn't really be using it)

It is not used by SDK. Only by server. But I agree... I should leave old constructor as deprecated.

We do not know everyone that uses this. The go.temporal.io/api library is one of the most included depended-upon libraries by Temporal users (granted usually transitively). Many users have proxies, client mocks, etc that may take advantage of these things. We should not adjust this library as if it's only used by server, https://github.com/temporalio/temporal is a better place for code that is only used by server.

cretz · 2025-05-14T13:11:13Z

serviceerror/already_exists.go

 	return &AlreadyExists{
 		Message: message,
 	}
 }

+// NewAlreadyExistsf returns new AlreadyExists error with formatted message.
+func NewAlreadyExistsf(format string, args ...any) error {


Users are not really expected to create these errors, do we really need new user-facing helpers? What is the use case driving this PR?

These errors are constructed mostly on server. User is a server dev. We have hundreds of usages with fmt.Sprintf and even small helpers like this. I want to get rid of all these helpers.

User of this library is more than server dev. I don't think we need to update this user-facing package with server-dev-only utilities. You aren't getting rid of the helpers, you're moving them to the this public, user-facing library for everyone. If the user is server dev, can it be put somewhere only for server dev?

Go SDK has several instances of serviceerror.New<Something>(fmt.Sprintf(...)), it can and should also use these new constructors.

Also, it would be pretty weird to have the NewSomething constructor be in this package but the NewSomethingf constructor be in a whole other package.

serviceerror package should NOT be in this repo at all. This is a mistake I made long time ago. Having constructor separate from struct definition doesn't look good to me. And yes, if other users use New<Something> they will definitely benefit from these new constructors too.

serviceerror package should NOT be in this repo at all. This is a mistake I made long time ago.

But it's used in the SDK and by users, what repo should it be in? Also, you have the ability to add these helpers in whatever repo you think, there's no requirement to add them in this one, so you don't have to perpetuate your mistake.

Go SDK has several instances of serviceerror.New(fmt.Sprintf(...)), it can and should also use these new constructors.

Right, I am not questioning the validity of the methods, I am questioning the user-facing aspect. These types of things are added in the Go SDK because we expect users to use them and therefore we are ok with the increased API surface area, stability guarantees, etc.

Also, it would be pretty weird to have the NewSomething constructor be in this package but the NewSomethingf constructor be in a whole other package.

I assume you mean weird to server developers, because it is not weird to everyone else that uses this library to not have those server-only helpers. We have to always keep users in mind, not just ourselves. It is very important.

My primary concern is adding server-only helpers to this repository. It is less of a concern with these of course since they are simple constructor overloads. But in general it's a bad pattern to add "server-developer-only helpers" to this user-facing library.

If y'all feel strongly enough about giving all users these helpers, ok (let me know via response), but we would prefer server-only code to be server-only code as a general rule (of course just some overloaded constructors is not that big of a deal).

They are not server-only. And yes, they should be here with the non-f constructors.

They are not server-only

This is at odds with some of the other comments, e.g. "It is not used by SDK. Only by server", hence the concern/thread here. Marking approved though I do think it's important to keep in mind that we should not clutter user packages with server-only needs if we can help it.

Not used by SDK yet 🙂

Add New...f constructors for all service errors

5d188fe

alexshtin requested review from a team as code owners May 12, 2025 21:56

dnr reviewed May 14, 2025

View reviewed changes

Address feedback

660083a

alexshtin requested a review from dnr May 14, 2025 07:30

cretz reviewed May 14, 2025

View reviewed changes

Return old NewAlreadyExist as deprecated

8e469a6

dnr approved these changes May 14, 2025

View reviewed changes

dnr mentioned this pull request May 15, 2025

Move a few occurrences of variables logged inline into tags temporalio/temporal#7767

Merged

5 tasks

cretz approved these changes May 15, 2025

View reviewed changes

Merge branch 'master' into feature/newf

ad01a5a

alexshtin merged commit 39aa8c9 into temporalio:master May 15, 2025
4 checks passed

alexshtin deleted the feature/newf branch May 15, 2025 23:45

		// NewAlreadyExistf returns new AlreadyExists error with formatted message.
		func NewAlreadyExistf(format string, args ...interface{}) error {

		// NewMultiOperationExecutionf returns a new MultiOperationExecution error with formatted message.
		func NewMultiOperationExecutionf(format string, errs []error, args ...interface{}) error {

Add New...f constructors for all service errors #221

Add New...f constructors for all service errors #221

Uh oh!

Conversation

alexshtin commented May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cretz May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cretz May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexshtin May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cretz May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cretz May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alexshtin commented May 12, 2025 •

edited

Loading

cretz May 14, 2025 •

edited

Loading

cretz May 14, 2025 •

edited

Loading

alexshtin May 14, 2025 •

edited

Loading

cretz May 15, 2025 •

edited

Loading

cretz May 15, 2025 •

edited

Loading