Skip to content
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

Enhancement: Allow initializing rows using a mapping #50

Closed
schveiguy opened this issue May 12, 2016 · 19 comments
Closed

Enhancement: Allow initializing rows using a mapping #50

schveiguy opened this issue May 12, 2016 · 19 comments

Comments

@schveiguy
Copy link
Contributor

schveiguy commented May 12, 2016

RowConvertible takes a Row object, and allows one to initialize an object based on the fields in the row.

However, you may fetch more than one object per row with a join. If you wanted to intialize two objects, you may rename some of the fields to avoid collisions. But the initializer is expecting certain names in the row to match what it is looking for.

A basic example is let's say you have 2 types of items. Item1 has at most one Item2. Each of these match objects that can initialize from a row. However, each one expects the field "id" to map to it's id. You can fetch all rows together from the DB, as one Row object, using a join. For example:

SELECT item1.id AS item2_id, item1.val AS item1_val,
               item2.id AS item2_id, item2.val AS item2_val
               FROM item1 LEFT JOIN item2 ON (item2.id = item1.item2_id)

But now, I cannot use the nice init function which takes a row to construct them, I have to use normal construction fetching the correctly named fields.

An adapter may be useful here, for example:

var adapter = ["id" : "item2_id", "val" : "item2_val"]
let item2 = Item2(row, withAdapter: adapter) // proposed usage?

Alternatively, you could make a Row constructor that uses another Row as backing with the adapter on top.

@groue
Copy link
Owner

groue commented May 13, 2016

@schveiguy Thanks for pushing the topic of associations.

When I read your question, it looks that the SQL query is built by some object (that fetches some items), and the adapter built by some other object (likely in the Item1 initializer). If so, the design looks brittle. Wouldn't it be better if the same object was building both the SQL query and the adapter?

Alternatively, you could make a Row constructor that uses another Row as backing with the adapter on top.

Yes, that's definitely a better fit.

Assuming I understand what you want, my first attempt at a solution would be the following: I introduce an Adapter type that has a main mapping, and mappings for "sub rows". Rows fetched with an adapter are seen through the main mapping. And rows can be asked for the eventual presence of sub rows:

let sql = "SELECT item1.id AS item2_id, item1.val AS item1_val, " +
          "item2.id AS item2_id, item2.val AS item2_val " +
          "FROM item1 LEFT JOIN item2 ON (item2.id = item1.item2_id)"

// Main mapping targets Item1. A sub row named "item2" targets Item2:
let adapter = Adapter(
    mapping: ["id" : "item1_id", "val" : "item1_val"],
    subRows: ["item2": ["id" : "item2_id", "val" : "item2_val"]])

// Fetch rows:
for row in Row.fetch(db, sql, adapter: adapter) {
    // The main mapping is active:
    row.value(named: "id")  // column "item1_id", actually

    // Row can be asked for the eventual presence of a "sub row":
    if let row2 = row.subrows["item2"] {
        row2.value(named: "id")  // column "item2_id", actually
    }
}

// Fetch item:
let items = Item1.fetchAll(db, sql, adapter: adapter)

RowConvertible types can still use their favorite column names in their initializers, and use sub rows if needed:

struct Item1: RowConvertible {
    let id: Int64
    let val: String
    let item2: Item2

    init(_ row: Row) {
        // Works both with and without adapter:
        id = row.value(named: "id")
        val = row.value(named: "val")

        guard let row2 = row.subrows["item2"] else {
            // Here Item1 really wants its item2.
            // But another logic could be implemented.
            fatalError("Missing item2")
        }
        item2 = Item2(row2)
    }
}

struct Item2: RowConvertible {
    let id: Int64
    let val: String

    init(_ row: Row) {
        // Works both with and without adapter:
        id = row.value(named: "id")
        val = row.value(named: "val")
    }
}

Would such a pattern fit your needs?

@groue
Copy link
Owner

groue commented May 13, 2016

Tests for a first draft: https://github.com/groue/GRDB.swift/blob/04e648b3f900e2a385d1cbd318ce257b82cae15c/Tests/Public/Core/Row/RowAdapterTests.swift

I still have questions about mapped row equality (row equality is important, especially for FetchedRecordsController), and what to do with mappings which refer to non-existing columns (should we crash, or not).

@schveiguy
Copy link
Contributor Author

If so, the design looks brittle.

I'm new to your library, and not an expert in this domain, so I'm going to defer to you in terms of what design works best. My real point is simply that I want to minimize the calls to the DB, and the initializer that accepts a Row is currently rigid in terms of what fields map to members.

In my concept, since RowConvertible (and not Persistent) needs something external to design/build the query and generate the rows, I did it how I would design a query to get such data :) I actually wasn't even thinking about initializing the Item2 inside the Item1 initializer, just the immediate need to be able to take some Row data, however it was created, and pass to Item2 initializer. But you are right that it's advantageous to be able to initialize Item1's Item2 instance inside Item1's initializer.

Would such a pattern fit your needs?

Definitely! I'm actually thinking about your answer to my previous question about an aggregate query (for an array), and see that subrows might be useful there as well.

I still have questions about mapped row equality

I have no experience with, and probably won't use for my application, the FetchedRecordsController, so I'm unsure how to respond to that.

and what to do with mappings which refer to non-existing columns

So you mean, you pass subrow to Item2, but subrow defines a mapping to a column not included in the actual row? Given that the subrow adapter and the row itself likely come from the same place, wouldn't this be a programming error?

@schveiguy
Copy link
Contributor Author

schveiguy commented May 13, 2016

I see what you mean in the first draft test about the missing id. I think this should crash, IMO, and probably upon the fetch.

Edit: reasoning is because missing id being nil may be an expected condition for instance if you have a subitem that sometimes is not present. How to distinguish this from poorly constructed query/adapter?

@groue
Copy link
Owner

groue commented May 13, 2016

Tell me more about your initial vision, if you don't mind.

@schveiguy
Copy link
Contributor Author

I can, but not today. I'm about to go off for the weekend, we can perhaps pick this up on Monday.

@schveiguy
Copy link
Contributor Author

So my initial vision for this enhancement was based on the model I have. There is one table that represents entities in a tree. Each entity can have zero or more attributes assigned to them. Each attribute is stored in another table with customized fields that define the attribute. Each entity can only have one attribute from a table, so the primary key is the same number as the entity primary key.

So I can fetch the entire entity description with one fetch using joins. If there isn't a matching attribute in that table, then its fields are nil.

However, it's possible that each attribute has fields named similarly to attributes in other tables. So in order to fetch them all, I may have to rename them.

But then, if I mark my actual attribute struct as RowConvertible, I have to either always fetch them with the renamed fields, rename the fields to something unique, or avoid the initializer that takes a row.

I wanted some way to utilize the row initializer, but to use the renamed fields needed to distinguish from other fields in the join.

@groue
Copy link
Owner

groue commented May 17, 2016

@schveiguy I see a mismatch between our views. My proposal won't expose the raw row (with the original column names) when it looks that yours does rely on them.

Actually, when I read your proposed code:

var adapter = ["id" : "item2_id", "val" : "item2_val"]
let item2 = Item2(row, withAdapter: adapter) // proposed usage?

... I wonder why you did not use a plain initializer:

struct Item2 {
    init(id: Int, val: String) { ... }
}
let item2 = Item2(id: row.value(named: "id"), val: row.value(named: "val"))

groue added a commit that referenced this issue May 18, 2016
groue added a commit that referenced this issue May 18, 2016
groue added a commit that referenced this issue May 18, 2016
groue added a commit that referenced this issue May 18, 2016
groue added a commit that referenced this issue May 18, 2016
groue added a commit that referenced this issue May 18, 2016
groue added a commit that referenced this issue May 19, 2016
@schveiguy
Copy link
Contributor Author

when it looks that yours does rely on them

No, I don't care to get the orginal row data fields. The less the initializer knows the better. The proposal was my poor attempt to add the ability to map the fields to the initializer. Your way is superior.

I wonder why you did not use a plain initializer:

Because the example is obviously just simple :) The true initializer may be more complex, and I don't want to have to construct some initializer tree so I can avoid code duplication. I just wanted to reuse the initializer I already have.

@groue
Copy link
Owner

groue commented May 19, 2016

OK 😄 I think that the support for row adapters is solid now. I just have to write some sensible documentation and bikeshed method names. Next time you'll need such a pattern, it will likely be ready in GRDB!

@schveiguy
Copy link
Contributor Author

thanks!

groue added a commit that referenced this issue May 20, 2016
@groue
Copy link
Owner

groue commented May 21, 2016

@schveiguy Here is the documentation: https://github.com/groue/GRDB.swift/tree/RowAdapter#row-adapters

What do you think? Shippable?

@schveiguy
Copy link
Contributor Author

Looked over the docs. Seems good!

@groue
Copy link
Owner

groue commented May 22, 2016

OK then: GRDB v0.67.0 is out, with support for row adapters. Thanks for your suggestion, it will help dealing with joined queries a lot!

@groue groue closed this as completed May 22, 2016
@groue
Copy link
Owner

groue commented May 23, 2016

Thanks for your request @schveiguy. Just used row adapters today, with much profit :-)

@groue
Copy link
Owner

groue commented May 23, 2016

@schveiguy Sorry to disturb you again. I'm happy with the feature, but not with the way things are named, after a little survey with my coworkers. I plan to ship a refactored version that, I hope, will be more clear. The revised version is at https://github.com/groue/GRDB.swift/tree/RowAdapter#row-adapters. Please share your comments if you have any.

@schveiguy
Copy link
Contributor Author

As I haven't yet had an opportunity to use it, the naming hasn't yet crossed my mind :)

I can see the problem with the name subrows doesn't really capture all possible uses for this feature, and mapping works quite well. I concur the naming change is a positive one.

@groue
Copy link
Owner

groue commented Jun 9, 2016

@schveiguy GRDB v0.72.0 has refactored row adapters. Check out the new documentation.

@groue
Copy link
Owner

groue commented Jul 8, 2016

@schveiguy GRDB v0.75.0 has refactored (again) row adapters. What was called "variant" is now called "scope", and I have good hope the API is stable now. See the new doc and the API diff.

cc @cfilipov who suggested this excellent renaming

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

No branches or pull requests

2 participants