Bkit 140 jpa #29

aochsner · 2017-12-19T18:46:04Z

Left todo:

Fix client (shape of json changed a bit...)
Let JPA manage schema (get rid of schema.sql)
Push more filtering in queries (going to be harder around interval overlap)
Ensure only "expand" one to many relationships when asked? (auto expand many to one for now?)

BKIT-140

case sensitivity when hibernate validates schema BKIT-140

Bit of WIP until booking works BKIT-140

BKIT-140

still needed for validations BKIT-140

aochsner · 2017-12-19T18:47:39Z

src/main/kotlin/com/buildit/bookit/v1/booking/dto/Booking.kt

+    @ManyToOne(optional = false)
+    val user: User,
+    @Id @GeneratedValue(generator = "uuid2") @GenericGenerator(name = "uuid2", strategy = "uuid2")
+    val id: String? = null


generally moved optional IDs to end

I slightly prefer a degree of isomorphism between the schema and the code, and like PKs first in a schema, but I can't what JPA will do in the schema anyway with regard to order (maybe it sorts keys to the top?).

fair point... i'll have a looksie as to what it actually does

Hibernate: drop table bookable if exists Hibernate: drop table booking if exists Hibernate: drop table location if exists Hibernate: drop table user if exists Hibernate: create table bookable (id varchar(255) not null, closed boolean not null, reason varchar(255) not null, name varchar(255) not null, location_id varchar(255) not null, primary key (id)) Hibernate: create table booking (id varchar(255) not null, end timestamp not null, start timestamp not null, subject varchar(255) not null, bookable_id varchar(255) not null, user_id varchar(255) not null, primary key (id)) Hibernate: create table location (id varchar(255) not null, name varchar(255) not null, time_zone varchar(255) not null, primary key (id)) Hibernate: create table user (id varchar(255) not null, external_id varchar(255) not null, family_name varchar(255) not null, given_name varchar(255) not null, primary key (id)) Hibernate: alter table bookable add constraint UK_6ietqnvn35wqiovo8qrmwk2ne unique (location_id, name) Hibernate: alter table location add constraint UK_sahixf1v7f7xns19cbg12d946 unique (name) Hibernate: alter table user add constraint UK_4eu2tvn9rj53a93fufx7ayr20 unique (external_id) Hibernate: alter table bookable add constraint FK1dc19oypan7r3ry418vn2h5sc foreign key (location_id) references location Hibernate: alter table booking add constraint FKtbhx4q4q4m1kv4muhynct3p81 foreign key (bookable_id) references bookable Hibernate: alter table booking add constraint FKkgseyy7t56x7lkjgu3wah5s3t foreign key (user_id) references user

so looks like it does the right thing i'd like to see from a schema perspective wrt order

aochsner · 2017-12-19T18:48:45Z

src/main/kotlin/com/buildit/bookit/v1/location/bookable/BookableController.kt

-    fun getBookable(@PathVariable("locationId") location: String, @PathVariable("bookableId") bookable: String): BookableResource {
-        locationRepository.getLocations().find { it.id == location } ?: throw LocationNotFound()
-        return BookableResource(bookableRepository.getAllBookables().find { it.id == bookable } ?: throw BookableNotFound())
+    fun getBookable(@PathVariable("locationId") location: Location?, @PathVariable("bookableId") bookable: Bookable?): BookableResource {


spring will automatically use the locationRepo to map locationId -> location object so we don't need our own dependency to it.

Magic is cool. And scary and confusing. Oh well, can't have everything :-)
And this, I think, answers my question above, re "witchcraft" (?).

aochsner · 2017-12-19T18:50:01Z

src/main/kotlin/com/buildit/bookit/v1/user/dto/User.kt

+    @Id @GeneratedValue(generator = "uuid2") @GenericGenerator(name = "uuid2", strategy = "uuid2")
+    val id: String? = null
+) {
+    val name get() = "$givenName $familyName"


sort of a hack to reuse the User entity as the User response but it still seems "simple enough"...

aochsner · 2017-12-19T18:50:43Z

src/main/resources/application.yml

+#    generate-ddl: false
+    show-sql: true
+    hibernate:
+      ddl-auto: validate


these settings might change once/if we let JPA manage the schema... for now still driven by schema.sql

I kind of fear us getting into a "deadlocked" deployment again if we fail to sync schema.sql and annotations and thus startup fails. You?

agree... hoping the existence of schema.sql is short term... next go around will eliminate it

aochsner · 2017-12-19T18:51:13Z

src/main/resources/schema.sql

-    USER_ID            VARCHAR(36) NOT NULL,
-    PRIMARY KEY        (USER_ID),
-    EXTERNAL_USER_ID   VARCHAR(128) NOT NULL,
+CREATE TABLE user (


msyql & hibernate schema validation seems to be case sensitive, although nothing else seems to be...

Weird. I actually like lower-case table names in case of MySql case-sensitive installation (hate having to type capital letter table names in queries).

aochsner · 2017-12-19T18:51:57Z

src/main/resources/schema.sql

-    PRIMARY KEY        (USER_ID),
-    EXTERNAL_USER_ID   VARCHAR(128) NOT NULL,
+CREATE TABLE user (
+    ID                 VARCHAR(36) NOT NULL,


took the naming conventions from JPA/Hibernate instead of adding to JPA annotations

aochsner · 2017-12-19T18:53:52Z

src/test-e2e/kotlin/com/buildit/bookit/v1/booking/BookingTests.kt

-                            "bookableId": "aab6d676-d3cb-4b9b-b285-6e63058aeda8",
+                            "bookable": {
+                                "id": "aab6d676-d3cb-4b9b-b285-6e63058aeda8"
+                            },


@frostiebot @lizziesz one example of how the shape of responses change...generally if you had an id coming back now you have a whole object (we only validate the "id" but everything comes back...)...

we can have further discussions as to whether this is OK or whether we shouldn't do that... or whether we should replace ids w/ URIs etc etc

This allows for flexibility in future and is better now before things harden more (IMO).

aochsner · 2017-12-19T18:54:19Z

src/test-e2e/kotlin/com/buildit/bookit/v1/location/bookable/BookableTest.kt

                                    "id": "aab6d676-d3cb-4b9b-b285-6e63058aeda8",
-                                    "locationId": "b1177996-75e2-41da-a3e9-fcdd75d1ab31",
+                                    "location": {
+                                        "id": "b1177996-75e2-41da-a3e9-fcdd75d1ab31"


@frostiebot @lizziesz ditto

aochsner · 2017-12-19T18:55:53Z

src/test/kotlin/com/buildit/bookit/v1/booking/BookingControllerMockMvcTests.kt

+                registry.addConverter(String::class.java, Booking::class.java) { id ->
+                    when (id) {
+                        BookingControllerMockMvcTests.booking.id -> BookingControllerMockMvcTests.booking
+                        else -> null


this magic is needed to handle the id -> object "lookup" that spring does for us now... almost decided this was too "magic" and went back to what we had... thoughts?

Witchcraft. Burn it.

Just kidding! Too disruptive to change back away from it based on the work you've done here, IMO. But my brain still recoils to getFoo(Foo).

mathomas

Much elegant. So better.
Thanks for your work on this!
My comments are mostly observations. See what you think, and merge away. Fire in the hole!

mathomas · 2017-12-21T15:47:58Z

src/main/kotlin/com/buildit/bookit/v1/booking/BookingController.kt

        return allBookings
            .filter { booking ->
-                val timezone = bookableTimezones[booking.bookableId] ?: throw IllegalStateException("Encountered an incomplete booking: $booking.  Not able to determine booking's timezone.")
+                val timezone = booking.bookable.location.timeZone


Changed due to constraints guaranteeing presence?

mathomas · 2017-12-21T15:51:38Z

src/main/kotlin/com/buildit/bookit/v1/booking/BookingController.kt

    @GetMapping("/{id}")
-    fun getBooking(@PathVariable("id") bookingId: String, @AuthenticationPrincipal user: UserPrincipal): Booking =
-        bookingRepository.getAllBookings().find { it.id == bookingId }.let { maskSubjectIfOtherUser(it ?: throw BookingNotFound(), user) }
+    fun getBooking(@PathVariable("id") booking: Booking?, @AuthenticationPrincipal user: UserPrincipal): Booking =


What witchcraft is this? Am looking at the diffs in the browser, so probably just need context.

mathomas · 2017-12-21T15:56:05Z

src/main/kotlin/com/buildit/bookit/v1/booking/dto/Booking.kt

+    @ManyToOne(optional = false)
+    val user: User,
+    @Id @GeneratedValue(generator = "uuid2") @GenericGenerator(name = "uuid2", strategy = "uuid2")
+    val id: String? = null


I slightly prefer a degree of isomorphism between the schema and the code, and like PKs first in a schema, but I can't what JPA will do in the schema anyway with regard to order (maybe it sorts keys to the top?).

mathomas · 2017-12-21T15:57:32Z

src/main/kotlin/com/buildit/bookit/v1/location/bookable/BookableController.kt

-    fun getBookable(@PathVariable("locationId") location: String, @PathVariable("bookableId") bookable: String): BookableResource {
-        locationRepository.getLocations().find { it.id == location } ?: throw LocationNotFound()
-        return BookableResource(bookableRepository.getAllBookables().find { it.id == bookable } ?: throw BookableNotFound())
+    fun getBookable(@PathVariable("locationId") location: Location?, @PathVariable("bookableId") bookable: Bookable?): BookableResource {


Magic is cool. And scary and confusing. Oh well, can't have everything :-)
And this, I think, answers my question above, re "witchcraft" (?).

mathomas · 2017-12-21T17:05:21Z

src/main/kotlin/com/buildit/bookit/v1/location/bookable/BookableController.kt

                    "bookings" in expand ?: emptyList() ->
-                        allBookings
-                            .filter { booking -> booking.bookableId == bookable.id }
+                        bookingRepository.findByBookable(bookable)


Now we have an O(n^2) (or some variety of such) thing here, I think, WRT database calls. With the data we have today, no big deal, but someday ...

yess..... thanks for point that out... forgot to clean this up... will leave in for this pr and fix right after

although, maaaaaybe some hibernate caching (all in the same transaction) alleviates us from being really dumb about it

I can't complain. Really nice work here. I'm loving the reduction in code. Spring Data is pretty darn nice.

mathomas · 2017-12-21T17:09:03Z

src/main/resources/application.yml

+#    generate-ddl: false
+    show-sql: true
+    hibernate:
+      ddl-auto: validate


I kind of fear us getting into a "deadlocked" deployment again if we fail to sync schema.sql and annotations and thus startup fails. You?

mathomas · 2017-12-21T17:12:23Z

src/test/kotlin/com/buildit/bookit/v1/booking/BookingControllerMockMvcTests.kt

+                registry.addConverter(String::class.java, Booking::class.java) { id ->
+                    when (id) {
+                        BookingControllerMockMvcTests.booking.id -> BookingControllerMockMvcTests.booking
+                        else -> null


Witchcraft. Burn it.

Just kidding! Too disruptive to change back away from it based on the work you've done here, IMO. But my brain still recoils to getFoo(Foo).

mathomas · 2017-12-21T17:15:43Z

src/test/kotlin/com/buildit/bookit/v1/location/bookable/BookableControllerMockMvcTests.kt

    }
+
+    @TestConfiguration
+    class InternalConfig {


Any possibility of DRY?

you mean between tests? although they'd have to share test data assumptions...

Location's mapping is defined in multiple tests. Just wonder if the @TestConfiguration can be DRY for that Entity, for example. It's not a major thing -- the code is write once (in a few locations), and never revisit.

It might be a stupid question, too. I'm really not familiar with Spring Data beyond the benefits, WRT repos, etc. The magical parts and implementation niceties are still a mystery.

i think it's for the test...although there mgiht be some ways to factor stuff out

BKIT-140

aochsner added 7 commits December 18, 2017 15:42

JPA: Location

612da0a

BKIT-140

JPA: Bookable

5be0eae

BKIT-140

Code cleanup

d2f45a2

JPA: mysql

2c40f07

case sensitivity when hibernate validates schema BKIT-140

JPA: User

36a3098

Bit of WIP until booking works BKIT-140

JPA: Booking + rest of User

e025d77

BKIT-140

Remove unnecessary field level annotations

2209298

still needed for validations BKIT-140

aochsner requested a review from mathomas December 19, 2017 18:46

aochsner commented Dec 19, 2017

View reviewed changes

mathomas approved these changes Dec 21, 2017

View reviewed changes

aochsner added 3 commits December 21, 2017 14:45

Merge branch 'master' into bkit-140-jpa

4bd6776

Merge branch 'master' into bkit-140-jpa

4a0a861

Merge master

ab4df26

BKIT-140

aochsner merged commit e81d62a into master Dec 21, 2017

aochsner deleted the bkit-140-jpa branch December 21, 2017 21:27

Bkit 140 jpa #29

Bkit 140 jpa #29

Uh oh!

Conversation

aochsner commented Dec 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mathomas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers