-
Notifications
You must be signed in to change notification settings - Fork 3k
Panache REST guide #9254
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
Panache REST guide #9254
Conversation
|
|
||
| == Resource customisation | ||
|
|
||
| Panache REST provides a `@PanacheRestResource` annotation that can be used to customize certain features of the resource. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's out of scope for this PR but I'm not happy with a name of this annotation (@PanacheRestResource). It's very similar to the interface name (PanacheCrud..Resource) and might be confusing. Any suggestions on the name change @geoand?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dunno.... I'm probably the worst developer when it comes to naming...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice start!
Added some comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some comments, some are related to the implementation itself rather than the doc.
/cc @geoand for further discussions.
| and pull requests should be submitted there: | ||
| https://github.com/quarkusio/quarkus/tree/master/docs/src/main/asciidoc | ||
| //// | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please avoid the newline here, the website is very picky about that.
|
|
||
| [source,java] | ||
| ---- | ||
| @Path("/entities") // Base bath is a pluralised name of an entity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a mistake. We shouldn't "pluralize" things as there's a good chance we will get some of them wrong and it really doesn't bring anything.
Keep in mind that a lot of people code in their own languages where the plural rules will be completely different.
Also, frankly, I don't think it's better as a plural so why take the risk?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/cc @FroMage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want plural, perhaps let the user specify it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to give an example, in French, we have a lot of specific plural forms (cheval -> chevaux for instance).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, taking the entity name as is would be the default and you could define a specific path in the annotation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just an FIY Spring Data REST simply adds s to every entity name.
Oh, my. OK I will create an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes it even more fun with:
@Entity
public class 馬 extends PanacheEntity {
}And the URI ends up being /馬s, right? :)
Myself, I've always used Cheval for the entity name (singular) and Chevaux (plural) for the controller name. And RoR derives the URIs from the controller names, so it would be /chevaux correctly.
I just suggest we advertise the same convention, it solves many issues. And we default to the controller name for the URI, instead of the entity name.
Naturally we should also allow users to override it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And the URI ends up being
/馬s, right? :)
I would think so. At least for a simple people I got a path /peoples when i tried it last time.
I just suggest we advertise the same convention, it solves many issues. And we default to the controller name for the URI, instead of the entity name.
Extracting a name from a controller will be trickier because a user can name his classes differently. We could recommend a convention but who knows if they'll follow it.
With an entity we could simply take an its name, make it all lowercase separated with - instead of camelcase and get on with it. And then I'll add an option to override the path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extracting a name from a controller will be trickier because a user can name his classes differently. We could recommend a convention but who knows if they'll follow it.
The point of conventions is that people follow them. If they don't they can't customise the output.
Entities are already singular by convention. Controller names don't have a convention other than the brain-dead "resource" that jax-rs got confused about.
It's pretty easy to tell people to name their controller People and default to using that name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's discuss it on #9261 since the guide change won't be enough for this anyway.
|
|
||
| === Available options | ||
|
|
||
| * `hal` - in addition to the standard `application/json` responses, generates additional methods that can return `application/hal+json` responses if requested via `Accept` header. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if we should have an array of content types instead with the default being { "application/json" }. That would be much more flexible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/cc @FroMage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I don't think we currently support any response type, but this seems like a valid point for the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
application/json is hardcoded and application/hal+json can be enabled as an extra. Because HAL is a complex format I had to implement a custom serialiser for it which serialisers special wrapper objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, let's keep it that way for now.
|
I think I've addresses all the comments except the one about plural paths. For that I'll make a separate guide modification once I change the extension code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great guide. Please don't hate me for reconsidering naming before it's too late…
| To begin with: | ||
|
|
||
| * Add the required dependencies to your `pom.xml` | ||
| ** Panache REST with Hiberante ORM extension (`quarkus-panache-rest-hibernate-orm`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that I read it like that, I've my doubts over the order, because originally it was supposed to be X with Panache, which means X with style.
This makes it looks like Panache is a noun and the subject instead of an adjective to make Hibernate ORM/REST stylish.
Perhaps it should be Hibernate ORM REST with Panache or RESTful Hibernate ORM with Panache or REST for Hibernate ORM with Panache?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess, similarly, this guide (and the extension names) would better be "REST with Panache" and rest-panache?
Except this sort-of implies it applies to REST apps that aren't necessarily about data, and we plan to have a Panache layer for RESTEasy one day…
Perhaps RESTful data with Panache?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or CRUD with Panache?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In any case, we need to make the extension name consistent with the naming rules we define here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I create a separate issue to agree on the name there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can, yes, but this will heavily impact this guide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could just update the guide together with the code once we agree on a name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
I've updated the guide to align with the changes from #9373 but I'll need to rebase after the merge. There will be a conflict in a |
|
We'll need a review from @FroMage for this one :) |
|
Closing this PR because I've cherry-picked the commit to #9373 |
cc @geoand