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

fixing join mapping - take 1 #410

Merged
merged 7 commits into from Mar 20, 2018

Conversation

2 participants
@rafiki270
Copy link
Contributor

rafiki270 commented Mar 20, 2018

I had to pass the info about the model down the generics chain ...

In the firstValue method I now first try to check for the "main" id and then have a fallback onto any "foreign" keys to the model. Obvious downside to this is that you won't be able to get a join field with the same name as you may have in the "main" table but its still an improvement me thinks ...

Sorry for the whitespaces, stupid copy/paste from my projects dependencies ... will remove if you confirm I am on the right path ...

@tanner0101
Copy link
Member

tanner0101 left a comment

Definitely on the right track, but have some ideas on how to make it even better. Thanks!

@@ -5,26 +5,26 @@ import Foundation
public final class QueryBuilder<Model, Result> where Model: Fluent.Model, Model.Database: QuerySupporting {
/// The query being built.
public var query: DatabaseQuery<Model.Database>

This comment has been minimized.

@tanner0101

tanner0101 Mar 20, 2018

Member

there's an xcode option to delete whitespace on empty lines. please enable that to reduce the git noise here :)

let q = self.query
let resultTransformer = self.resultTransformer
return connection.flatMap(to: Void.self) { conn in
return Model.Database.execute(query: q, into: { row, conn in
resultTransformer(row, conn).map(to: Void.self) { result in
return try handler(result)
}.catch { error in
print("[Fluent] Skipping row: \(error)")
}.catch { error in

This comment has been minimized.

@tanner0101

tanner0101 Mar 20, 2018

Member

formatting needs fixing here

@@ -43,21 +43,33 @@ extension QueryField: ExpressibleByStringLiteral {

extension Dictionary where Key == QueryField {
/// Accesses the _first_ value from this dictionary with a matching field name.
public func firstValue(forField fieldName: String) -> Value? {
public func firstValue(forField fieldName: String, on entity: String) -> Value? {

This comment has been minimized.

@tanner0101

tanner0101 Mar 20, 2018

Member

this "firstValue" should stay the same (no on entity label). Use value(forEntity entity: String, atField field: String) instead. A QueryField with same table name and entity is the same query field, so it doesn't make sense to say "first" value for a specific field name and entity--it's the "only value"

return try D.init(from: decoder)
}
}

/// MARK: Private
fileprivate final class _QueryDataDecoder<Database>: Decoder where Database: QuerySupporting {
fileprivate final class _QueryDataDecoder<M>: Decoder where M: Model, M.Database: QuerySupporting {

This comment has been minimized.

@tanner0101

tanner0101 Mar 20, 2018

Member

I think I'd prefer to keep this generic on the Database and just pass in entity: String during init so that this can be used for non-model types (very common) as well!

@rafiki270 rafiki270 force-pushed the rafiki270:fixing-join-mapping branch 2 times, most recently from 1801910 to dd6ebf6 Mar 20, 2018

@rafiki270 rafiki270 force-pushed the rafiki270:fixing-join-mapping branch 2 times, most recently from 97be58b to 8e7688a Mar 20, 2018

@rafiki270 rafiki270 force-pushed the rafiki270:fixing-join-mapping branch from 8e7688a to ea2a78b Mar 20, 2018

rafiki270 added some commits Mar 20, 2018

@tanner0101
Copy link
Member

tanner0101 left a comment

this is perfect, thank you @rafiki270!

@tanner0101 tanner0101 added this to the 3.0.0-rc.2 milestone Mar 20, 2018

@tanner0101 tanner0101 self-assigned this Mar 20, 2018

@tanner0101 tanner0101 changed the base branch from nio to master Mar 20, 2018

tanner0101 and others added some commits Mar 20, 2018

@rafiki270

This comment has been minimized.

Copy link
Contributor Author

rafiki270 commented Mar 20, 2018

@Tanner now it should be good to go, I had to test in my app first to find all still works

@tanner0101 tanner0101 merged commit 666352d into vapor:master Mar 20, 2018

0 of 4 checks passed

ci/circleci: linux Your tests are queued behind your running builds
Details
ci/circleci: linux-mysql Your tests are queued behind your running builds
Details
ci/circleci: linux-postgresql Your tests are queued behind your running builds
Details
ci/circleci: linux-sqlite Your tests are queued behind your running builds
Details

@rafiki270 rafiki270 deleted the rafiki270:fixing-join-mapping branch Mar 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment