Add join type awareness to models and data stores #982

nerrad · 2019-02-26T02:13:12Z

Note: given a high priority because this is a blocker for REM work.

Problem this Pull Request solves

As @tn3rb has been working on implementing the CRUD work from the models/data-stores into the REM work he is doing, he's encountering difficulties with:

efficient network queries (so getting all the tickets for all the datetimes attached to an event in as few queries as possible - as opposed to doing a getRelatedTickets request for each datetime).
using the existing selectors for the eventespress/core in an expected manner despite the efficient queries.

The difficulty lies in the fact that retrieving and setting up all the relations in the store state for entities retrieved through efficient queries is a extensive process that's easy to get "wrong" because of the opinionated way relation data is kept in the state.

The purpose of this pull then is to introduce some changes so that:

BaseEntity models currently are aware of the possible relations, but are not aware of the relation types. We need to expose this in the models so efficient queries can be built.
Alternatively (and what might be the better option) is eventespresso/schema can be used for deriving the relation types and relation models from the schema stored in its state. Then there could simply be selectors that get the needed data for building the appropriate queries and knowing what dispatches to execute to get the correct relations setup in eventespresso/core.
eventespresso/core will have a selector (and resolver) for getting all the related entities for a collection of entity ids. So for example getRelatedEntitiesForIds( datetimeIds, 'ticket' ). This would handle all the necessary dispatches etc for correctly adding the relations to the store state as well as using the new selectors in eventespresso/schema to know if relations have to be joined via a join table query.
I'd like to also explore if the schema (and the REST API behaviour) provides enough information to programmatically derive the most efficient endpoint to do the query on. For instance, since the include parameter allows for requesting related model entities to be returned in the response, it might be possible to get all the entities and their relations in one go via the join table (or base model). If that is the case, then the number of requests can be reduced more and it might even make the relation dispatches easier.

Changes in this pull

Along with the purpose outlined in the original post, I created a test app in #988 based off of this branch to do some implementation testing of things to ensure it worked as expected. While testing, there were a number of other fixes that had to be done.

Along with all the below, unit-tests were updated, new tests added, and additional developer documentation was created as well.

New features

added resolveRelationRecordForRelation action generator (eventespresso/core)
added getRelatedEntitiesForIds selector and resolver. (eventespresso/core)
added new receiveRelationSchema action and related reducer to eventespresso/schema store.
added new hasJoinTableRelation selector and resolver (eventespresso/schema).
added new getRelationType selector and resolver (eventespresso/schema).
added new getRelationSchema selector and resolver (eventespresso/schema).
added a new clone getter to the BaseEntity class. Can be used to clone a given entity instance.
added a new forClone property to the BaseEntity class. Can be used to export a plain object representing all the fields and values on a given entity instance that can immediately be used in instantiating a new entity instance.

Fixes

Fix incorrect argument list in the receiveEntityAndResolve dispatch control invoked in the createRelation action. ( eventespresso/core)
Add normalization for model and relation names in the createRelations action. (eventespresso/core)
Ensure the the getEntityById and getRelationEntities selectors are resolved for the new relations created in the createRelations action. (eventespresso/core)
add some fixes to the eventespresso/core reducer for relations: make sure id is always normalized.
make sure model name is normalized for getSchemaForModel resolver in eventespresso/schema

How has this been tested

Existing unit tests
New unit tests for new functionality.
Via the demo app in Fet/demo copy date functionality #988 which is utilizing the all the new features introduced in this pull as well as testing the various side-effects related to state changes caused by new relations etc.

Checklist

I have read the documentation relating to systems affected by this pull request, see https://github.com/eventespresso/event-espresso-core/tree/master/docs
User input is adequately validated and sanitized
all publicly displayed strings are internationalized (usually using esc_html__(), see https://codex.wordpress.org/I18n_for_WordPress_Developers)
My code is tested.
My code follows the Event Espresso code style.
My code has proper inline documentation.
My code accounts for when the site is in Maintenance Mode (MM2 especially disallows usage of models)

nerrad · 2019-02-26T02:14:24Z

For example's sake, here's an example of the kind of thing necessary now to do efficient queries (code @tn3rb has been using in current development):

export default createHigherOrderComponent(
    compose( [
        withDatesListFilterState,
        withTicketsListFilterState,
        withSelect( ( select, ownProps ) => {
            let eventDates = [];
            let eventDateTickets = [];
            let eventDateIds = [];
            let eventDateTicketMap = {};
            const coreStore = select( 'eventespresso/core' );
            const listStore = select( 'eventespresso/lists' );
            const event = coreStore.getEventById( ownProps.eventId );
            if ( isModelEntityOfModel( event, 'event' ) ) {
                eventDates = coreStore.getRelatedEntities( event, 'datetimes' );
                eventDates = condenseArray( eventDates );
                if ( ! isEmpty( eventDates ) ) {
                    eventDateIds = getDatetimeEntityIds( eventDates );
                    const queryString = 'where[Datetime.DTT_ID][IN]=[' +
                        eventDateIds.join( ',' ) + ']';
                    const ticketQueryString = queryString +
                        '&default_where_conditions=minimum';
                    eventDateTickets = listStore.getEntities(
                        'ticket',
                        ticketQueryString
                    );
                    eventDateTicketMap = buildEventDateTicketRelationsMap(
                        eventDateIds,
                        eventDateTickets,
                        listStore.getEntities( 'datetime_ticket', queryString )
                    );
                }
            }
            return { event, eventDates, eventDateTickets, eventDateTicketMap };
        } ),
    ] ),
    'withDatesAndTicketsFilterState'
)( DatesAndTicketsFilterState );

tn3rb

looks great but had a couple of really minor questions/suggestions

tn3rb · 2019-03-04T19:03:11Z

assets/src/data/eventespresso/core/resolvers/relations.js

+		modelName,
+		relationName,
+	);
+	const relationSchema = yield resolveSelect(


should we do a hasJoinTable check before proceeding any further?
ie: what happens if the following function calls run (resolveSelect(), getPrimaryKey(), etc) when no join exists?

EDIT
nevermind... i see now that this is checking for a many to many intersection table and not just any table join

tn3rb · 2019-03-04T19:10:31Z

assets/src/data/eventespresso/core/resolvers/relations.js

+				! isArray( relationRecords ) ?
+				[ relationRecords ] :
+				relationRecords;
+			while ( relationRecords.length > 0 ) {


it looks to me like relationRecords could still potentially be null at this point,
so might want to add an Array.isArray() check or similar protection to avoid "Cannot read property 'length' of null" errors

great catch! Ya I'll just check it isn't null before running the while.

tn3rb · 2019-03-04T19:26:48Z

assets/src/data/eventespresso/core/resolvers/relations.js

+				}
+			}
+		}
+	} else {


the blocks of logic in this if else statement are large enough that they could benefit from being extracted into their own function which would make this more readable.
ie:

if ( hasJoinTable ) { getManyRelations(); } else { getSingularRelations() }

The issue here is that this is a generator so your examples would have to be generators also (which increases the testing maintenance load as well). With this only occurring in this method for now (and the components not being reused anywhere yet ) I'd prefer to wait on extracting things until future potential use demonstrates a pattern. For instance the pattern that might get extracted is the actual while loops. I think its premature to extract this yet.

Worth noting, all the logic from the resolveRelationRecordForRelation action used to be in this resolver as well! So I already did do some extraction :)

…ries

- fixes a bug with state not updating correctly for entity map.

…dispatch

- returns values of object usable for cloning via a factory.createNew call.

… values

… new relation.

- tests needed updates due to changes in defaults and fixtures for other tests.

nerrad · 2019-03-04T22:09:52Z

@tn3rb ready for review. The change up for review is just this commit: 55ac44e

@tn3rb

…type awareness to models and data stores (#982) ## Problem this Pull Request solves As @tn3rb has been working on implementing the CRUD work from the models/data-stores into the REM work he is doing, he's encountering difficulties with: - efficient network queries (so getting all the tickets for all the datetimes attached to an event in as few queries as possible - as opposed to doing a getRelatedTickets request for each datetime). - using the existing selectors for the `eventespress/core` in an expected manner despite the efficient queries. The difficulty lies in the fact that retrieving and setting up all the relations in the store state for entities retrieved through efficient queries is a extensive process that's easy to get "wrong" because of the opinionated way relation data is kept in the state. The purpose of this pull then is to introduce some changes so that: - `BaseEntity` models currently _are aware_ of the possible relations, but are _not_ aware of the relation types. We need to expose this in the models so efficient queries can be built. - Alternatively (and what might be the better option) is `eventespresso/schema` can be used for deriving the relation types and relation models from the schema stored in its state. Then there could simply be selectors that get the needed data for building the appropriate queries and knowing what dispatches to execute to get the correct relations setup in `eventespresso/core`. - `eventespresso/core` will have a selector (and resolver) for getting all the related entities for a collection of entity ids. So for example `getRelatedEntitiesForIds( datetimeIds, 'ticket' )`. This would handle all the necessary dispatches etc for correctly adding the relations to the store state as well as using the new selectors in `eventespresso/schema` to know if relations have to be joined via a join table query. - I'd like to also explore if the schema (and the REST API behaviour) provides enough information to programmatically derive the most efficient endpoint to do the query on. For instance, since the `include` parameter allows for requesting related model entities to be returned in the response, it might be possible to get all the entities and their relations in one go via the join table (or base model). If that is the case, then the number of requests can be reduced more and it might even make the relation dispatches easier. ## Changes in this pull Along with the purpose outlined in the original post, I created a test app in #988 based off of this branch to do some implementation testing of things to ensure it worked as expected. While testing, there were a number of other fixes that had to be done. Along with all the below, unit-tests were updated, new tests added, and additional developer documentation was created as well. ### New features * added `resolveRelationRecordForRelation` action generator (`eventespresso/core`) * added `getRelatedEntitiesForIds` selector and resolver. (`eventespresso/core`) * added new `receiveRelationSchema` action and related reducer to `eventespresso/schema` store. * added new `hasJoinTableRelation` selector and resolver (`eventespresso/schema`). * added new `getRelationType` selector and resolver (`eventespresso/schema`). * added new `getRelationSchema` selector and resolver (`eventespresso/schema`). * added a new `clone` getter to the `BaseEntity` class. Can be used to clone a given entity instance. * added a new `forClone` property to the `BaseEntity` class. Can be used to export a plain object representing all the fields and values on a given entity instance that can immediately be used in instantiating a new entity instance. ### Fixes * Fix incorrect argument list in the `receiveEntityAndResolve` dispatch control invoked in the `createRelation` action. ( `eventespresso/core`) * Add normalization for model and relation names in the `createRelations` action. (`eventespresso/core`) * Ensure the the `getEntityById` and `getRelationEntities` selectors are resolved for the new relations created in the `createRelations` action. (`eventespresso/core`) * add some fixes to the `eventespresso/core` reducer for relations: make sure id is always normalized. * make sure model name is normalized for `getSchemaForModel` resolver in `eventespresso/schema` "

nerrad added type:feature-request 🙏 category:assets category:models-and-data-infrastructure priority:high labels Feb 26, 2019

nerrad self-assigned this Feb 26, 2019

nerrad force-pushed the FET/add-relation-type-awareness-to-models-and-stores branch from f7e86c3 to cee6e50 Compare February 27, 2019 01:16

nerrad mentioned this pull request Feb 27, 2019

Fet/demo copy date functionality #988

Closed

nerrad force-pushed the FET/add-relation-type-awareness-to-models-and-stores branch from 66ab4e0 to 6637af0 Compare March 1, 2019 16:34

nerrad requested a review from tn3rb March 3, 2019 23:01

nerrad assigned tn3rb Mar 3, 2019

nerrad marked this pull request as ready for review March 3, 2019 23:17

tn3rb reviewed Mar 4, 2019

View reviewed changes

nerrad added 17 commits March 4, 2019 16:19

started todo so can create pull

416b796

add new selectors/actions etc to schema for relation type related que…

7748cc7

…ries

add new selector for doing efficient relation entity query

7b40402

fix incorrect arg for dispatch

c99a6c7

ensure entityId is normalized

3b9067a

- fixes a bug with state not updating correctly for entity map.

fix bugs due to incorrectly prepared strings

22b3eb5

make sure we use Immutable.Map and we’re passing the right values on …

f790298

…dispatch

fix bugs with prepped strings used as args

cfbf71a

add clone getter to BaseEntity

2539348

add forClone getter to BaseEntity

00072fd

- returns values of object usable for cloning via a factory.createNew call.

add field extractor for fields and values used for cloning

8aa09d8

Fix incorrect arguments

7c7d89d

make sure new clone method is exporting all enumerable properties and…

ff596d8

… values

add missing semiColon

73b7adf

add missing resolving of getRelatedEntities when creating a relation

ce9a6fa

handle doing reverse resolution of getRelatedEntities when creating a…

c470d9c

… new relation.

fix another bug and update existing tests for changes

42dc3ff

nerrad added 5 commits March 4, 2019 16:19

add jsdocs and unit-test converage

b05281a

add dev docs for new things

bd82d97

update tests and fix linting

d90cfbe

- tests needed updates due to changes in defaults and fixtures for other tests.

account for relationRecords having a null value

55ac44e

update build dev files

b4c542b

nerrad force-pushed the FET/add-relation-type-awareness-to-models-and-stores branch from ab1fe3e to b4c542b Compare March 4, 2019 22:07

nerrad requested a review from tn3rb March 4, 2019 22:09

tn3rb approved these changes Mar 4, 2019

View reviewed changes

tn3rb removed their assignment Mar 4, 2019

nerrad added this to the 4.9.80.p milestone Mar 5, 2019

nerrad merged commit 70e538a into master Mar 5, 2019

joshfeck removed the priority:high label Apr 29, 2019

joshfeck deleted the FET/add-relation-type-awareness-to-models-and-stores branch August 16, 2019 20:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add join type awareness to models and data stores #982

Add join type awareness to models and data stores #982

Uh oh!

nerrad commented Feb 26, 2019 •

edited

Loading

Uh oh!

nerrad commented Feb 26, 2019

Uh oh!

tn3rb left a comment

Uh oh!

tn3rb Mar 4, 2019

Uh oh!

tn3rb Mar 4, 2019

Uh oh!

nerrad Mar 4, 2019

Uh oh!

tn3rb Mar 4, 2019

Uh oh!

nerrad Mar 4, 2019

Uh oh!

nerrad commented Mar 4, 2019

Uh oh!

Uh oh!

Add join type awareness to models and data stores #982

Add join type awareness to models and data stores #982

Uh oh!

Conversation

nerrad commented Feb 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem this Pull Request solves

Changes in this pull

New features

Fixes

How has this been tested

Checklist

Uh oh!

nerrad commented Feb 26, 2019

Uh oh!

tn3rb left a comment

Choose a reason for hiding this comment

Uh oh!

tn3rb Mar 4, 2019

Choose a reason for hiding this comment

Uh oh!

tn3rb Mar 4, 2019

Choose a reason for hiding this comment

Uh oh!

nerrad Mar 4, 2019

Choose a reason for hiding this comment

Uh oh!

tn3rb Mar 4, 2019

Choose a reason for hiding this comment

Uh oh!

nerrad Mar 4, 2019

Choose a reason for hiding this comment

Uh oh!

nerrad commented Mar 4, 2019

Uh oh!

Uh oh!

nerrad commented Feb 26, 2019 •

edited

Loading