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

Web Actions Tab: Add null handling to Application/Network Interceptors #844

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@adrw
Copy link
Member

adrw commented Mar 12, 2019

  • Application or Network Interceptors are appearing null when they lack a qualified class name
  • New ClassNameFormatter that handles formatting a valid class name
@wesleyk

This comment has been minimized.

Copy link
Collaborator

wesleyk commented Mar 12, 2019

let's add a test

@adrw adrw force-pushed the adrw:adrw/20190312.WebActions16 branch 3 times, most recently from c3b4e46 to d484dac Mar 12, 2019

@@ -72,8 +73,8 @@ internal fun WebActionMetadata(
parameterTypes = parameterTypes.map { it.toString() },
returnType = returnType.toString(),
pathPattern = pathPattern.toString(),
applicationInterceptors = applicationInterceptors.map { it::class.qualifiedName.toString() },
networkInterceptors = networkInterceptors.map { it::class.qualifiedName.toString() },
applicationInterceptors = applicationInterceptors.map { ClassNameFormatter().format(it::class) },

This comment has been minimized.

@wesleyk

wesleyk Mar 12, 2019

Collaborator

let's inject this dependency

}
}

fun isValid(className: String): Boolean {

This comment has been minimized.

@wesleyk

wesleyk Mar 12, 2019

Collaborator

do we use this? Why expose it?

@@ -27,6 +29,12 @@ class WebActionMetadataActionTest {
assertThat(customServiceActionMetadata.pathPattern).isEqualTo("/custom_service_access")
assertThat(customServiceActionMetadata.allowedServices).containsOnly("payments")
assertThat(customServiceActionMetadata.allowedRoles).isEmpty()
assertThat(
customServiceActionMetadata.applicationInterceptors.all {
classNameFormatter.isValid(it)

This comment has been minimized.

@wesleyk

wesleyk Mar 12, 2019

Collaborator

seems a bit backwards to assert this here; how about instead we confirm that we return the formatted class name?

@wesleyk
Copy link
Collaborator

wesleyk left a comment

nice! thanks for adding the test. Few comments about how to restructure the dependency and removing the unused API (with a suggestion on what to use for assertions instead)

@adrw adrw force-pushed the adrw:adrw/20190312.WebActions16 branch 7 times, most recently from 700ae4d to 4aff3d9 Mar 12, 2019

Web Actions Tab: Add null handling to Application/Network Interceptors
* Application or Network Interceptors are appearing null when they lack a qualified class name
* New `ClassNameFormatter` that handles formatting a valid class name

@adrw adrw force-pushed the adrw:adrw/20190312.WebActions16 branch from 4aff3d9 to db3041c Mar 12, 2019

@@ -139,6 +141,10 @@ class MiskWebModule(private val config: WebConfig) : KAbstractModule() {
install(WebActionModule.create<ReadinessCheckAction>())
install(WebActionModule.create<LivenessCheckAction>())
install(WebActionModule.create<NotFoundAction>())

// WebActionMetadataAction.Factory
bind<ClassNameFormatter>().toInstance(ClassNameFormatter())

This comment has been minimized.

@wesleyk

wesleyk Mar 12, 2019

Collaborator

we shouldn't need these bindings

@@ -30,6 +31,44 @@ class WebActionMetadataAction @Inject constructor() : WebAction {
}

data class Response(val webActionMetadata: List<WebActionMetadata>)

class Factory {

This comment has been minimized.

@wesleyk

wesleyk Mar 12, 2019

Collaborator

rather than an additional layer of abstraction let's have a top level method on the action

WIP
@adrw

This comment has been minimized.

Copy link
Member Author

adrw commented Mar 14, 2019

ClassNameFormatter pulled out into #851

@adrw

This comment has been minimized.

Copy link
Member Author

adrw commented Mar 17, 2019

Merged with #853

@adrw adrw closed this Mar 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.