add optional QueryBuilder

[garak]

See ddeboer/data-import#230

[sagikazarmark]

This class is getting more and more complex. @Baachi shouldn't we split it into more parts?

[Baachi]

@sagikazarmark Split this big class in some smaller classes is a good idea.

[sagikazarmark]

One possible way is to split by query origin: DQL, QueryBuilder. In the end, they all produce an iterable result.

[sagikazarmark]

Looking at #1 it seems to me that internally a query builder will be used.

Also, looking at the proposed constructor: does the objectName parameter make any sense if a query builder is passed as a third argument?

So my idea is to be able to set a query for the reader after it is initiated, for example:

• setQueryFor('Object\Name')
• setQueryBuilder(<queryBuilder>)

[garak]

The idea is to reserve the third parameter for more advanced needs. So, if you simply need to get all entities, you can go with objectName; if you instead need something more complex, you need to pass the QueryBuilder

[sagikazarmark]

My problem is that you can pass both, but in case you pass the query builder, object name is not needed. Why would you need to pass it then? It is not clear this way.

[garak]

You need it anyway in getFields method

[sagikazarmark]

No, you don't. You can simply fetch the first result and get field names from it.

[garak]

It looks to me like an ugly hack

[sagikazarmark]

Look at other repos: it is used generally. Also: passing an object name AND a query builder is redundant and you can easily get into an inconsistent state.

[garak]

Well, if other repos are wrong, I don't see a reason for being wrong: it's better to take fields info from metadata.
We could add a check for consistency in querybuilder object, but the approach in querybuilder is that such checks are out of its responsibility

[sagikazarmark]

Even if you add a check, redundancy is still bad. Requireing redundant parameters is simply bad. I don't see why it is bad to determine fields based on fetched data. getFields are always called after rewind.

[garak]

Redundancy is a price for flexibility.
Fetching data when you can avoid fetching is a waste.

[sagikazarmark]

Redundancy is a price for flexibility.

What flexibility?

Fetching data when you can avoid fetching is a waste.

You don't actually fetch data, since it is already fetched. getFields is ALWAYS called after fetching data. You can consider getFields an internal method. (Which should be removed IMHO, or moved to a separate interface @Baachi?) Also by letting devs passing a simple query builder without an object you get bigger flexibility.

[Baachi]

First of all i would only allow a QueryInterface, this is more flexible for users who didn't use the query builder.

The thing is, it's not really easy to implement that. If you pass a query instance to it, which use joins or something similar or a subquery in the select clause it will fail,
because you only get the metadata from the passed object or from the first result. Why not pass simple an Criteria object? This should bring more flexibilty to this reader.

[sagikazarmark]

Actually I think getFields should entirely be removed from Reader interface. If we are going to implement some kind of Item object, we don't need a separate method for field names in the reader. Also, by removing redundancy, you either pass in an object name or a query. In that case, field names can be determined based on fetched data and passed directly to the item object. However criteria is a good idea, when combined with the object name version. One question though: does ODM support QueryInterface?

[ddeboer]

Replaced with #12.