Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WAL 202 did exceptions #777

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

WAL 202 did exceptions #777

wants to merge 6 commits into from

Conversation

SuperBatata
Copy link
Contributor

@SuperBatata SuperBatata commented Sep 27, 2024

Description

Provide a clear and concise description of the changes made in this pull request:
Enhance did exceptions with custom exceptions with meaningful error messages

Type of Change

  • bug fix - change which fixes an issue
  • new feature - change which adds functionality

Checklist

  • code cleanup and self-review
  • unit + e2e test coverage
  • documentation updated accordingly

Breaking

  • -

@SuperBatata SuperBatata marked this pull request as ready for review September 30, 2024 07:35
Copy link

sonarcloud bot commented Oct 1, 2024

@@ -65,17 +69,18 @@ data class ServiceMap(
) {

init {
require(id.isNotBlank()) { "Service property id cannot be blank" }
require(id.isNotBlank()) { throw InvalidServiceIdException("Service property id cannot be blank") }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

require already throws an IllegalArgumentException and within that lambda function you provide the message which the exception should be constructed with

image

so, what happens when you throw within the lambda lazyMessage function itself is:

  1. require calls lazyMessage() to retrieve the message / object to get passed along with the exception
  2. during that call it fails
  3. and throws the exception that was constructed within the lazyMessage function (hence never finishing the assignment to message and not proceeding with the rest of the flow)

an option would be to create a custom require method, e.g.:

private fun require(condition: Boolean, type: KType, callback: () -> String) {
    if (!condition) {
        throw createException(type, callback())
    }
}

private fun createException(type: KType, message: String) = when (type) {
    typeOf<InvalidServiceIdException>() -> InvalidServiceIdException(message)
    typeOf<InvalidServiceTypeException>() -> InvalidServiceTypeException(message)
    typeOf<EmptyServiceEndpointException>() -> EmptyServiceEndpointException(message)
    else -> IllegalArgumentException(message)
}

then call it, e.g.:

init {
    require(id.isNotBlank(), typeOf<InvalidServiceIdException>()) { "Service property id cannot be blank" }
    type.forEach {
        require(it.isNotBlank(), typeOf<InvalidServiceTypeException>()) { "Service type strings cannot be blank" }
    }
    require(
        serviceEndpoint.isNotEmpty(), typeOf<EmptyServiceEndpointException>()
    ) { "Service endpoint set cannot be empty" }
    customProperties?.forEach {
        require(!reservedKeys.contains(it.key), typeOf<ReservedKeyOverrideException>()) {
            "Invalid attempt to override reserved Service property with key ${it.key} via customProperties map"
        }
    }
}

in pure jvm, require could use more reflection, e.g.:

private fun require(condition: Boolean, clazz: KClass<Throwable>, callback: () -> String) {
    if (!condition) {
        val constructor = clazz::class.primaryConstructor
        throw constructor!!.call(callback()).objectInstance!!
    }
}

but I'm not sure smth. similar is available in multiplatform
however, neither one is an elegant solution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants