"ById" is not taking empty resource name into account

[yuranos]

In NamingHelper:

                if (index > 0 && index == splitUrl.length-1) {
                        String peek = splitUrl[index-1].toLowerCase();
                        if (segment.toLowerCase().contains(NamingHelper.singularize(peek))) {
                            //this is probably the Id
                            name = name + "ById";
                        } else {
                            name = name + "By" + StringUtils.capitalize(segment.substring(1, segment.length()-1));
                        }
                    }

peek can be empty and I'm sure it would be preferable for everyone to have "By" + StringUtils.capitalize(segment.substring(1, segment.length()-1));

As an example: For raml:

/customers/{customerId}/bookings:
  /{bookingRef}:
    description: Booking details entity
    get:
      description: Get the booking details with `bookingRef = {bookingRef}`
      queryParameters:
        siteKey : SiteKey

I'd rather have:

    @RequestMapping(value = "/{bookingRef}", method = RequestMethod.GET)
    public ResponseEntity<BookingDetailsDto> getByBookingRef(
        @PathVariable
        String bookingRef,
        @PathVariable
        String customerId,
        @RequestParam
        SiteKey siteKey);

Then:

    @RequestMapping(value = "/{bookingRef}", method = RequestMethod.GET)
    public ResponseEntity<BookingDetailsDto> getById(
        @PathVariable
        String bookingRef,
        @PathVariable
        String customerId,
        @RequestParam
        SiteKey siteKey);

Now, there's another issue with the way resource.getUri() is built, but I will report it separately.

[stojsavljevic]

Hi,

so it's done on purpose assuming that path variable is an id which is natural to assume for REST API. If you have:

/abc/bookings:
    /{bookingId}:

method generated will be getById which is IMHO a bit better than getByBookingId since we are returning Booking object.

What we can improve is recognizing an id as a path variable so we keep getById in that case and we use your logic in all other cases.

Another thing I don't like is that generated method name depends on other parts of resource - /abc in my example since without it generated method name will be getBookingById. Maybe I miss some logic here. @kurtpa can you enlighten us on the naming logic?

[yuranos]

HI, @stojsavljevic . For your example, it doesn't make sense to call a path param bookingId, we can elaborate on that in README. My case is about something different. BookingRef is not the same as bookingId. I know it might not comply with RESTful conventions, but as with #211, it's all about adding flexibility. Unlike with some of my old PRs where I added new maven plugin params, and you rightfully stated that too much flexibility can be potentially a minefield, in this case, everything is hidden from a plugin user. Even with the extra verbosity that you mentioned in your example, I think it's better than having a confusing "update" naming for POST.

[yuranos]

Another thing I don't like is that generated method name depends on other parts of resource - /abc in my example since without it generated method name will be getBookingById.

Sorry, @stojsavljevic , I didn't get it. /abcwill never be taken into account. This name = name +is a complete mess and I deleted it as part of the PR. That part of the code will only be called when index == splitUrl.length-1, so it seems there's no need to append the name to itself.

[stojsavljevic]

I tested this on your branch:

/bookings:
  get:
    responses: 
      200:
        body: 
          application/json:
            type: Book[]
  /{bookingRef}:
    uriParameters: 
      bookingRef: number
    get:
      responses: 
        200:
          body: 
            application/json:
              type: Book

gives:

BookingController

public ResponseEntity<Book> getBookingById(BigDecimal bookingRef);

while this:

/customers/{customerId}/bookings:
  uriParameters: 
    customerId: number
  get:
    responses: 
      200:
        body: 
          application/json:
            type: Book[]
  /{bookingRef}:
    uriParameters: 
      bookingRef: number
    get:
      responses: 
        200:
          body: 
            application/json:
              type: Book

produces:

BookingCustomerIdCustomerController

public ResponseEntity<Book> getByBookingRef(BigDecimal bookingRef, BigDecimal customerId);

And raml snippets are almost the same! Also, this:

/bookings:
  /abc/{bookingId}:

will lead to different results but this:

/bookings/abc:
  /{bookingId}:

So, the way resources are organized influences creation of controller and the name of methods. Don't worry, this is existing behavior, not something you break up. I pointed this out so anyone is aware.

What you want to change in your PR is the first case where you still get ById suffix. I will be perfectly fine if you add ById only in case when path variable is exactly (but case insensitive comparison) resource name (singular) + id. Basically, this is the line you want to change:

if (!StringUtils.isEmpty(peek) && segment.toLowerCase().contains(NamingHelper.singularize(peek))) {

And we need few tests to cover this issue :) Create something like Issue215RulesTest using few cases like ones we just discussed.

[yuranos]

@stojsavljevic , I don't know what we are doing differently, but using my branch I get a completely different result. For both RAMLs you provided, I get getByBookingRef method name and I also get the same controller name every time, BookingController.

[yuranos]

@stojsavljevic , I added Issue212RulesTest, please check. I understand the method names you provided, I think, but I still can't get how you generated BookingCustomerIdCustomerControllercontroller name.

[stojsavljevic]

Sorry again for the confusion with file names - I explained it in #217

But when it comes to method names (which is the subject of the issue) - I still see different behaviors. And your tests confirm that (issue-212-2.raml and issue-212-1.raml). When you have root resource /bookings - generated method name is getBookingById. With something in front of /bookings resource (in your test it's /customers/{customerId}) - method name is getByBookingRef.

[yuranos]

@stojsavljevic , please check my last commit. I just realised that what I really didn't like and what was really confusing is the way the URL was reduced. I actually wanted to report it a long time ago, but now I think maybe we can simply fix it in scope of this ticket. Just think of it: modelling RESTful contracts should be very neat and straightforward. The minimum URL should be something like resource/{id}. And there's simply no reason to call id something like uri_int_param. To me getActionName looks a lot clearer now. What do you think? Just the fact that there's only 5 tests failing in my last commit is telling me that the solution is pretty good and does not introduce a lot of distraction. The code is more linear now and less cyclomatically complex. And how about the old method name vs a new one: getByBookingRef vs getBookingByRef

• not bad?

[stojsavljevic]

I just realised that what I really didn't like and what was really confusing is the way the URL was reduced. I actually wanted to report it a long time ago, but now I think maybe we can simply fix it in scope of this ticket.

Can you give me an example? Do you need some more work for this?

Just think of it: modelling RESTful contracts should be very neat and straightforward. The minimum URL should be something like resource/{id}. And there's simply no reason to call id something like uri_int_param.

Agree that contracts should be neat and straightforward. But as we discussed, there more than one way to do things. Some people might use resource/{id} and some resource/{resourceId}. I'd like to have both cases boil down to the same generated code. I think that's true with your latest commit. Am I right?

To me getActionName looks a lot clearer now. What do you think?

Well.. It's not very clear but it was even worse before. So.. 👍

Just the fact that there's only 5 tests failing in my last commit is telling me that the solution is pretty good and does not introduce a lot of distraction.

That's definitely a good thing. Can we fix remaining test please?

And how about the old method name vs a new one: getByBookingRef vs getBookingByRef

Much better indeed 👍

[yuranos]

@stojsavljevic , you are right. With my latest commit both resource/{id}and resource/{resourceId}. Unfortunately, I can't get any credit for that, because it's all under the hood of StringUtils.difference() :laughing:

[yuranos]

Tests fixed. Give it a look, @stojsavljevic .

[yuranos]

Hello, @stojsavljevic . Are you there? We haven't lost you for Xmas, have we?:)

[stojsavljevic]

Hi @yuranos

no, you haven't lost me for Xmas, I'm Orthodox and my Xmas is in January :) Actually, I'm working hard on the plugin - I'm working on a simplification as you can see in Project page.

I will try to merge your PR today, but as stated in #65, I will work more to consolidate naming before next release.