Spring-boot: Provide additional IDE-friendly handler details in the mappings endpoint

[wilkinsona]

The STS team would like to be able to use the response from the mappings endpoint to navigate from a mapping to the code that will handle requests made to it. We should provide additional details in the response that contains the information in a predictable format when it's available. Hopefully @kdvolder can offer some input on what that format should be.

[wilkinsona]

@YannCebron We'd welcome input from the IDEA side of things too if you have a moment. @AlexFalappa I'm not sure if you do anything with the mappings endpoint in NetBeans, but please let us know your thoughts if you do.

[kaleev]

Mappings endpoint output already contains handler methods' signatures in both SB 1.5 and 2.0.
It is enough for Intellij IDEA to perform navigation to the handler's source code.

[kdvolder]

For folks who weren't in on some of the discussions we already had on this on email. We came to an aggreement that even though its impossible to define the format for all cases (seeing as the framework makes it pretty open-ended) common cases that are of interest are when a handler corresponds to a method or possibly a bean.

The main purpose of this would be to give hint to an IDE of where the handler is defined in the code (i.e. if you did something like 'Goto Definition' in an IDE or 'smart' editor, then it could open something meaningful and place the cursor there).

As Andy proposed we could leave the current 'handler' field as it is (essentially its a String not particularly meant to be parsed by anyone but may convey useful information to a human reader) and add some extra optional fields with well-defined and parseable format for specific cases.

To open Java code I think two common cases of where we'd want to point to could be:

1. a class
2. a method inside a class.

So I propose adding two optional fields class and method.

The class field points to a fully qualified typename. Simply using the value of java.lang.Class.getName() seems suitable.

The method field further narrows the location to a specific method within the class. As such it should only be added together with a corresponding class field.

The format I propose for identifying a specific method is as follows:

  "method" : {
      "name": ...method-name...
      "descriptor": ...method descriptor...
  }

For constructor the special method-name <init> is used.

The descriptor follows the format as defined in the JVM bytecode specification. See here for details: https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-4.html#jvms-4.3.3

I don't think toString value of a java.lang.Method object is suitable. Allthoug it does have enough information to identify a method, it's format isn't clearly defined and so its not clear how to parse it properly. The method descriptor on the other hand is formally defined and there are libraries available for parsing and generating it (e.g ASM to name one).

There may be things other than java classes and methods that could be considered as 'where a handler is defined' (e.g. for static resource handler, it may be associated with a file or folder on classpath) but just focussing on the method/class cases is good place to start for now.

[AlexFalappa]

@wilkinsona the NB Spring Boot plugin does not exploit any actuator endpoint at the moment, I have filed #89 to keep track of this enhancement but it is not something I will tackle in the near future.

[kdvolder]

Sorry I overlooked something... method descriptor alone is not sufficient to identify a method. So I went back and changed my proposal above a little (making method a json object/map and adding a 'name' field).

[kdvolder]

@kaleev said:

Mappings endpoint output already contains handler methods' signatures in both SB 1.5 and 2.0.
It is enough for Intellij IDEA to perform navigation to the handler's source code.

I think that's true. STS is also able to do this. But it is worrysome that the code doing this on our end feels rather hacky and it is clear the string values attached to 'method' field in Boot 1.5 and 'handler' field in Boot 2.0 don't seem to be intended for anyone to parse (yet that is what we do). I think moving forward we can do better by coming to a clear agreement / specification.

[wilkinsona]

Here's a proposal that adds some details extracted from the HandlerMethod for dispatcher servlet mappings that are RequestMappingInfoHandlerMapping instances:

{
  "contexts" : {
    "application" : {
      "mappings" : {
        "dispatcherServlets" : {
          "dispatcherServlet" : [ {
            "handler" : "ResourceHttpRequestHandler [locations=[class path resource [META-INF/resources/], class path resource [resources/], class path resource [static/], class path resource [public/], ServletContext resource [/], class path resource []], resolvers=[org.springframework.web.servlet.resource.PathResourceResolver@ea3e784]]",
            "predicate" : "/**/favicon.ico"
          }, {
            "handler" : "public java.lang.Object org.springframework.boot.actuate.endpoint.web.servlet.AbstractWebMvcEndpointHandlerMapping$OperationHandler.handle(javax.servlet.http.HttpServletRequest,java.util.Map<java.lang.String, java.lang.String>)",
            "predicate" : "{[/actuator/mappings],methods=[GET],produces=[application/vnd.spring-boot.actuator.v2+json || application/json]}",
            "details" : {
              "handlerMethod" : {
                "className" : "org.springframework.boot.actuate.endpoint.web.servlet.AbstractWebMvcEndpointHandlerMapping.OperationHandler",
                "name" : "handle",
                "signature" : "(Ljavax/servlet/http/HttpServletRequest;Ljava/util/Map;)Ljava/lang/Object;"
              }
            }
          }, {
            "handler" : "protected java.util.Map<java.lang.String, java.util.Map<java.lang.String, org.springframework.boot.actuate.endpoint.web.Link>> org.springframework.boot.actuate.endpoint.web.servlet.WebMvcEndpointHandlerMapping.links(javax.servlet.http.HttpServletRequest,javax.servlet.http.HttpServletResponse)",
            "predicate" : "{[/actuator],methods=[GET],produces=[application/vnd.spring-boot.actuator.v2+json || application/json]}",
            "details" : {
              "handlerMethod" : {
                "className" : "org.springframework.boot.actuate.endpoint.web.servlet.WebMvcEndpointHandlerMapping",
                "name" : "links",
                "signature" : "(Ljavax/servlet/http/HttpServletRequest;Ljavax/servlet/http/HttpServletResponse;)Ljava/util/Map;"
              }
            }
          }, {
            "handler" : "ResourceHttpRequestHandler [locations=[class path resource [META-INF/resources/webjars/]], resolvers=[org.springframework.web.servlet.resource.PathResourceResolver@7c710d98]]",
            "predicate" : "/webjars/**"
          }, {
            "handler" : "ResourceHttpRequestHandler [locations=[class path resource [META-INF/resources/], class path resource [resources/], class path resource [static/], class path resource [public/], ServletContext resource [/]], resolvers=[org.springframework.web.servlet.resource.PathResourceResolver@6dff5a55]]",
            "predicate" : "/**"
          } ]
        },
        "servletFilters" : [ {
          "servletNameMappings" : [ ],
          "urlPatternMappings" : [ "/*" ],
          "name" : "requestContextFilter",
          "className" : "org.springframework.boot.web.servlet.filter.OrderedRequestContextFilter"
        }, {
          "servletNameMappings" : [ ],
          "urlPatternMappings" : [ "/*" ],
          "name" : "httpPutFormContentFilter",
          "className" : "org.springframework.boot.web.servlet.filter.OrderedHttpPutFormContentFilter"
        }, {
          "servletNameMappings" : [ ],
          "urlPatternMappings" : [ "/*" ],
          "name" : "hiddenHttpMethodFilter",
          "className" : "org.springframework.boot.web.servlet.filter.OrderedHiddenHttpMethodFilter"
        } ],
        "servlets" : [ {
          "mappings" : [ ],
          "name" : "default",
          "className" : "org.apache.catalina.servlets.DefaultServlet"
        }, {
          "mappings" : [ "/" ],
          "name" : "dispatcherServlet",
          "className" : "org.springframework.web.servlet.DispatcherServlet"
        } ]
      }
    }
  }
}

Note the new details section with contents like this:

"details" : {
  "handlerMethod" : {
    "className" : "org.springframework.boot.actuate.endpoint.web.servlet.AbstractWebMvcEndpointHandlerMapping.OperationHandler",
    "name" : "handle",
    "signature" : "(Ljavax/servlet/http/HttpServletRequest;Ljava/util/Map;)Ljava/lang/Object;"
  }
}

Useful?

[kdvolder]

Yes, that looks useful but... some comments.

1) One difference between what I proposed is that this only supports (or shows how to) point to a method inside a class. Could it not be useful to also point to a class by itself?

As pointing to a method kind of implies pointing to the class the method is inside of, in my proposal, I tried to cover both cases by making the properties that narrow down the class scope to a method as optional and dependent on the presence of class property.

Even if you don't think this makes sense (i.e. maybe you'd assume a handler never corresponds to a class) I still think it makes sense to support pointing to just a class. E.g. when, for whatever reason you cannot pinpoint or determine the exact method, it may still be possible to identify a class. I.e. in the philosophy of 'best effort' its better to point to the general area of a class containing a handler definition than pointing to nothing.

2) Another thought... the example you posted may not be the best to judge how useful it is.

The main use-case we probably care about is what comes out when a user defines their own endpoints via something like a @RestController and @GetMapping, @PutMapping etc.

If those will end up showing the handlerMethod as pointing to AbstractWebMvcEndpointHandlerMapping.OperationHandler.handle() then its not very useful at all.

[kdvolder]

@wilkinsona Is the example you showed just a 'mockup' or is there a real implementation behind it. If the latter, I wouldn't mind giving it a try to see what kind of data comes out of different examples request handler definitions that I throw at it. Is there snapshot or branch I can somehow use to try it out?

[kdvolder]

Thought a bit about your proposal @wilkinsona I'd rework it slightly to the following:

"handlerDetails" : {
    "className" : "org.springframework.boot.actuate.endpoint.web.servlet.AbstractWebMvcEndpointHandlerMapping.OperationHandler",
  "method" : {
    "name" : "handle",
    "signature" : "(Ljavax/servlet/http/HttpServletRequest;Ljava/util/Map;)Ljava/lang/Object;"
  }

Differences with your example.

1) changed details to handlerDetails. Reason: we will probably also eventually want a predicateDetails as the predicate is also something we already parse. We can take it one step at a time and leave this for later, but anticipating this already, it is probably good to distinguish handlerDetails from predicateDetails to avoid awkard names (or need for renaming) in the future.

2) pulled the classname out of the method info. I.e. we consider the method info in the context of the class info and allow class info to be provide without any method info. I'm not saying you have a case where you need to generate that (I don't know). But its more of an understanding/promise on our end that STS will not 'blow-up' if you only tell us a class and not the exact method in the class where the handler is defined.

[kdvolder]

Another thought... in the interest of compactness. The 'method' could be a string which is the concatenation of name and signature. As there can't be a '(' in a method name its very easy to split the string at the right spot anyway. So there isn't much benefit to putting this into two separate fields.

The example then becomes:

"handlerDetails" : {
  "className" : "org.springframework.boot.actuate.endpoint.web.servlet.AbstractWebMvcEndpointHandlerMapping.OperationHandler",
  "method" : "handle(Ljavax/servlet/http/HttpServletRequest;Ljava/util/Map;)Ljava/lang/Object;"
}

[philwebb]

Even if you don't think this makes sense (i.e. maybe you'd assume a handler never corresponds to a class) I still think it makes sense to support pointing to just a class. E.g. when, for whatever reason you cannot pinpoint or determine the exact method, it may still be possible to identify a class. I.e. in the philosophy of 'best effort' its better to point to the general area of a class containing a handler definition than pointing to nothing.

I can't think of any situation where we can find the class but not the method. I'd rather we aim for a precise reference, rather than make things fuzzy.

If those will end up showing the handlerMethod as pointing to AbstractWebMvcEndpointHandlerMapping.OperationHandler.handle() then its not very useful at all.

That's just the method that is used by actuator endpoints. Use defined endpoints would point to the @GetMapping etc method.

[wilkinsona]

Thanks for taking a look, @kdvolder.

Is the example you showed just a 'mockup' or is there a real implementation behind it.

I'm working on these changes in this branch.

The 'method' could be a string which is the concatenation of name and signature.

This feels to me like it would be a small step back towards what we have in 1.5. One of the main motivations for these changes is to remove the need to split things back up and to minimise custom parsing. For that reason, I'd rather keep the name and signature separate in the JSON. The signature is, as suggested, produced by ASM and should, therefore, be in a well-defined and predictable format.

changed details to handlerDetails.

It's probably not clear from the example, but the intention is for details to be a single place beneath which all implementation-specific details can be placed. This makes it easier for clients that don't care to ignore those details and easier for clients that do care to find those details. If/when we add support for describing implementation details beyond a HandlerMethod other entries will be added to the details map alongside handlerMethod.

Even if you don't think this makes sense (i.e. maybe you'd assume a handler never corresponds to a class) I still think it makes sense to support pointing to just a class. E.g. when, for whatever reason you cannot pinpoint or determine the exact method, it may still be possible to identify a class. I.e. in the philosophy of 'best effort' its better to point to the general area of a class containing a handler definition than pointing to nothing.

I can't think of any situation where we can find the class but not the method. I'd rather we aim for a precise reference, rather than make things fuzzy.

Agreed. As I said above, this example is specifically for a HandlerMethod where we'll always have a Method. If there's a class-only case it'll be for something other than a HandlerMethod and we can consider it in due course. The functional web stuff with a lambda could be one such case. As described above, this separate case will appear in a separate entry in the details map.

[kdvolder]

HandlerMethod other entries will be added to the details map alongside handlerMethod.

Okay, I understand. You are looking at it from the perspective of who is doing the pointing. I'm just coming at it from a slightly different perspective and asking the question 'what are we (able to) point at'. When it comes to Java code the number of things you could point at is rather limited. I.e. method's fields, classes (and possibly line numbers). I didn't want to really go into the cases that don't matter yet so that's why I stuck to 'method' and 'class' (because class is implied by method).

My thoughts were that if you come at it from the 'what do we point at' angle you may have fewer cases in general because different kinds of things could all be mapped to a method, class, field. And you wouldn't have to define new named 'details' fields to represent them. I.e hypothetically, let's say you have some way you could define a 'handler' via some kind of a factory method. Would you need to add a 'handlerFactoryMethod' because, technically speaking its not a 'handlerMethod' (even though the action you'd want when a user asks 'open the definition in the code' in both cases is 'open a method inside a class', in both cases)

Anyhow, I don't feel too strongly about it. These are just my thoughts. Admittedly, perhaps its hard to see which is the better approach given that we only have one concrete example (handlerMethod).

It might be beneficial to think about some other examples. Maybe this one:

{
            "handler" : "ResourceHttpRequestHandler [locations=[class path resource [META-INF/resources/], class path resource [resources/], class path resource [static/], class path resource [public/], ServletContext resource [/]], resolvers=[org.springframework.web.servlet.resource.PathResourceResolver@6dff5a55]]",
            "predicate" : "/**"
          } ]

Would it make sense if that one pointed to a class called ResourceHttpRequestHandler as its 'place where it is defined'? Or would it make more sense if it pointed instead to something else (maybe a resource / folder in the file system).

BTW: I'm not saying that my way of thinking about it (define different things to point at, rather than define details for every different type of handler that could exist) necessarily better. I'm actually not sure. But it seems like there is a choice to be made here, which is why it might be good if he had some other examples to consider. If we can't find any other good examples. Then perhaps it doesn't matter much and we can stick with whatever seems simpler / better to you to solve the case we care about the most right now (i.e. handlerMethod) and deal with other cases if/when they come up.

[kdvolder]

It's probably not clear from the example, but the intention is for details to be a single place beneath which all implementation-specific details can be place

Fair enough. If you want to stuff everything into one field I do not object to that.

So maybe something like his then (if you along go with my way of thinking about defining cases around 'pointing to different kinds of locations of definition'.

"details" : {
   "definitionLocation" : {
       "class" : "com.demo.MyRestController",
       "method" : {
          "name" : "hello",
          "descriptor": "()Ljava.lang.String;"
      }
    }
}

About concatenating method name and descriptor.

This feels to me like it would be a small step back towards what we have in 1.5.

Separate is fine. I don't really have a preference. Just proposed that as another option in case the 'compactness' of the json seems important (But I guess it itsn't the main consideration to neither of us).

[wilkinsona]

So maybe something like his then (if you along go with my way of thinking about defining cases around 'pointing to different kinds of locations of definition').

Thanks for the suggestion, but I don't like this idea. I want the structure of the response to be as predictable a possible. If we combine multiple different types of handlers into a definitionLocation, what will appear in that part of the response will be very hard to predict and anything that wants to consume it will have to have numerous logic branches.

My preference is for every entry in the details section to have a predictable and consistent structure. The only logic that will then be required on the client side is to see if a particular piece of the details is there. If it isn't, you can move on. If it is, you can consume it knowing exactly what structure it will have.

Would it make sense if that one pointed to a class called ResourceHttpRequestHandler as its 'place where it is defined'? Or would it make more sense if it pointed instead to something else (maybe a resource / folder in the file system).

We could probably list the locations with which the ResourceHttpRequestHandler is configured in a way that's more easily consumed. However, it gets tricky quite quickly. The locations are implementations of org.springframework.core.io.Resource. This is somewhat similar to the problem that we have with HandlerFunction in WebFlux as it means that the implementation's free to do pretty much whatever it wants and that makes it impossible to describe accurately in all cases.

[kdvolder]

I want the structure of the response to be as predictable a possible.

My preference is for every entry in the details section to have a predictable and consistent structure

I agree, and that is why we are trying to define a 'contract' on what we would expect inside of 'definitionLocation' irrespective of the type of handler that contributes information into that optional attribute.

In other words I would rather like to see us describe the kinds of structures and IDE can expect in definitionLocation irrespective of the type of handler that contributes it. Then any type of handler is free to contribute a piece of information consistent with that structure into that attribute. The information is optional so there is no obligation on a handler to provide the information. But... it it does then the IDE will provide some extra functionality (a link or action to open the handler's definition location in a editor).

I don't see how this makes the structure more complex or unpredictable, seeing as would explictly describe the exact structure to expect and its meaning and then leave it up to different types of hanlders to optionaly implement to contribute that... or not.

On the other hand if I understand your way of thinking then every different type of handler will serialize to a different attribute with a potentially different structure. Which in my view makes things more unpredictable. From my point of view at least, because, I don't really care about all the different types of handlers, all I really want to know is, if a user asks to open the definition, what should I open for them in the editor. It doesn't matter if this was a handlerFactoryMethod, a handlerMethod, a handlerBean defined in a configuration class's method etc. As long as the thing to be openened is a method, to me its really all the same case.

We could probably list the locations with which the ResourceHttpRequestHandler is configured in a way that's more easily consumed. However, it gets tricky quite quickly.

Okay, let's just not go there yet and stick to handlerMethod for now. There's just one thing I want to react to.

the implementation's free to do pretty much whatever it wants and that makes it impossible to describe accurately in all cases.

I think we really have to get away from the idea that it has to describe accurately in all cases. Instead we should consider a clear purpose for a 'details' attribute and ask what is that attribute supposed to be used for. Define its meaning and layout in that sense, and then, it is totally up to the handler implementation if they want to bother providing information that fits that attribute's mold. If its too hard, or it doesn't seem to apply to that type of handler, then the handler is free not contribute that information.

Anyhoo... at this point, maybe it might be good if we have a face-to-face? It might be more efficient that way to come to conclusion and reconcile our different points of view, more forward and get something useful implemented. Or we can continue back-and-forth a bit more on this here...

Let me know what you prefer (face-to-face vs continue discussion here) and I'll try to setup a meeting if that's what you want (and of course anyone who's interested would be invited to participate).

[wilkinsona]

In other words I would rather like to see us describe the kinds of structures and IDE can expect in definitionLocation irrespective of the type of handler that contributes it.

As we've already discussed above, I don't think that's an achievable goal due to all the different types of handlers.

It doesn't matter if this was a handlerFactoryMethod, a handlerMethod, a handlerBean defined in a configuration class's method etc. As long as the thing to be openened is a method, to me its really all the same case.

As we've already discussed, there are cases where there is no method.

I'd much rather deal with differences in the details at the highest possible level (immediately nested beneath details in this case) rather than at arbitrary, more deeply nested places in the response. My experience with REST Docs and the APIs that people have documented with it is that this approach results in a far easier to document and far easier to use API. It also makes it much easier for us to add additional details in the future as they'd simply be a new entry immediately beneath details.

we should consider a clear purpose for a 'details' attribute and ask what is that attribute supposed to be used for

I think the purpose is already clear. The goal is to provide additional, implementation-specific details about a handler. Those details should be in a format that is IDE-friendly (the use of ASM to describe method signatures for example), but should not be specifically tailored to consumption by an IDE. The Actuator's intended to provide production-ready features so it's already debatable that we should be concerned about IDEs at all. If what we provide is generally useful while also being IDE-friendly then I think we will have succeeded.

We can continue to discuss this if needs be, but time is running out, particularly for what is a low priority issue. We discussed this at today's Boot team meeting and were unanimous in the belief that the approach that I have described above is the right one to take. From what you've said, STS will be able to consume the format that we're proposing and it has some significant benefits in terms of flexibility, predictability, and extensibility.

[philwebb]

We've been bitten hard in the past by trying to second guess future requirements and making things too lenient, I'm don't think we should repeat that mistake again. A handlerMethod _is_ different from a ResourceHttpRequestHandler and it _is_ different from a Functional Web lambda. We shouldn't try to represent them in the same data structure.

I think the current suggestion works very well. If you have a "handlerMethod" section in the JSON you know you'll have a class, method and signature you can use. If we start making any of those things optional all users need to remember that they might disappear in a future version. That's a big source of potential bugs because something you rely on suddenly isn't there you get a null pointer exception.

Sure, the design also means that future additions won't be immediately supported in all IDEs, but I'd rather take that and have stability.

[kdvolder]

I think the purpose is already clear. The goal is to provide additional, implementation-specific details about a handler.

To me that is not a clear goal at all unless you focus the thoughts around what the details are meant to be used for. If you don't ask the question what the details will or could be used for then its just a somewhat arbitrary collection of information which may or may not serve any specific purpose.

I grant you, that's still better than having the same information in a 'unparsable' toString value though. So it is progress.

it's already debatable that we should be concerned about IDEs at all.

I see, well that is a somewhat disapointing stance. If that's the case I suppose there is no further point continuing the discussion as then you are totally right, a collection of 'random details' not intended for any specific purpose seems to be the goal you have in mind. So in that view, I see it makes no sense to focus on my request to add information to support a style 'open the location' request specifically.

Anyway, your proposal is already progress. It is definitely better to have a 'handlerMethod' than it is to try and guess whether the string in 'handler' looks like a method and parse it with some add hoc parser and keep once fingers crossed that fragile mess won't break in the future.

So let's just move ahead with this then and implement that. It is progress and it adresses the problem for an important case. So its a useful step. Sorry if I dragged on the discussion too much. I guess I just misunderstood what was meant by 'IDE friendly'.

There is another painpoint the proposal does not address, but its probably independent so I'll raise another ticket for that shortly.

[wilkinsona]

it's already debatable that we should be concerned about IDEs at all.

I see, well that is a somewhat disapointing stance.

That would be a disappointing stance, but it's not the stance of the Boot team. You've selectively quoted what I wrote and changed its meaning. In the following sentence I said the following:

If what we provide is generally useful while also being IDE-friendly then I think we will have succeeded.

In other words, this feature should not be designed specifically for consumption by an IDE. The goal is to produce something that's generally useful while also being IDE friendly.

[kdvolder]

The goal is to produce something that's generally useful while also being IDE friendly.

Okay, yes. That's fine. Like I said I just misunderstood what you meant by 'IDE friendly'. I understand better where you are coming from now. Basically, you meant something more like 'parsable' rather than specifically intended to support an IDE. So basically, I'm a bit disapointed as I was hoping something more focussed. But it is still useful though and we should move ahead with that. I'll refrain in future from framing discussion around 'to support this IDE function' but stick to simply asking for details we care about to be made 'parsable'.

[martinlippert]

Let me add a few (low priority) thoughts here... :-)

First of all, thanks to @wilkinsona for working on these refinements and additions to make the data more IDE friendly while keeping the goal of being generally useful in mind. Since the actuator endpoints are not meant to be designed only and specifically for IDE consumption, this makes total sense to me.

While reading this thread again, I came back to a previous thoughts that I had a while ago when thinking about the actuator usage from our side: What do you (@wilkinsona , @philwebb) think about us implementing our own IDE-specific actuator endpoints (in addition to what is already provided by boot out of the box)? Just a thought, but would love to hear your thoughts on that.

[wilkinsona]

What do you think about us implementing our own IDE-specific actuator endpoints

I'd prefer to avoid going down that route if at all possible. Providing IDE-specific endpoints would require the IDE to add those endpoints to the application, changing its runtime behaviour. I don't like that. When I'm writing an application, I want the code that runs in my IDE to be as close as possible to what will run when built and tested on the command line.

[kdvolder]

I agree with Andy on this one. For the reasons that Andy gave and also because maintaining code that 'injects' this functionality into a user's app somehow, is likely a bit of a maintenance nightmare and a source of weird bugs when it malfunctions.