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

perf(posts): save some of the multiple db queries performed to fetch … #3907

Merged
merged 5 commits into from
Apr 8, 2020

Conversation

tuxpiper
Copy link
Member

@tuxpiper tuxpiper commented Apr 1, 2020

…geojson from posts

This pull request makes the following changes:

  • pre-fetches point and geometry values along with the mean post search query
  • avoids queries into tables other than Post if their results are not going to be needed (as is the case for the geojson controller)

Test checklist:

  • Perform usual map, data and post browsing operations

  • I certify that I ran my checklist

Fixes ushahidi/platform# .

Ping @ushahidi/platform

@tuxpiper tuxpiper requested a review from rowasc April 1, 2020 15:09
@tuxpiper
Copy link
Member Author

tuxpiper commented Apr 1, 2020

This is the first batch of database query optimizations for very popular requests to Platform API , such as fetching GeoJSON.

There's still a pretty obvious optimization pending to implement, in order to avoid repeatedly querying for some form table attributes. Leaving that for another PR as I'm looking into clean (and safe) ways to store that information in memory for the duration of the request/job.

@coveralls
Copy link

coveralls commented Apr 1, 2020

Coverage Status

Coverage decreased (-0.2%) to 20.309% when pulling 72b5e71 on geojson-optimization into 7783a1b on develop.

@@ -40,7 +40,8 @@ public function prepBoundingBox(Request $request)
protected function getFilters(Request $request)
{
return parent::getFilters($request) + [
'include_types' => ['point', 'geometry']
'include_types' => ['point', 'geometry'],
'output_core_post' => true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may be good to add a comment on this new filter's meaning

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added it here:

https://github.com/ushahidi/platform/pull/3907/files#diff-15be46c5155c0769e0fb9dd020f43de8R493

since that's where all the filters are anyway... Shall I duplicate it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, I think that's fine then. Probably something we should have in a static const but not a worry.

Copy link
Contributor

@rowasc rowasc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good so far. I think maybe a caught a typo?
This is pretty confusing in general (not your fault, this was all already pretty confusing) so it took some time to go through, sorry . Beyond pre existing tests, I'd ask Walter to focus on permissions and data security while testing. I'll also give it another read because at some point it got pretty confusing to follow all the bits, but I'd prefer we fix any urgent issues now while we keep looking at it and schedule testing right away (rather than block testing) to catch anything critical.

$attrs = $this->form_repo->getAllFormStagesAttributes($form_ids);

$this->form_attributes_by_form = $attrs->groupBy('form_id');
$this->form_attributes_by_key = $attrs->keyBy('key');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks like a typo maybe? (keyBy)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah no, that's a Collection method , it works 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That takes the form attribute queries result and creates a new collection indexed by attribute key

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, fancy. Ok !

Copy link
Contributor

@leninpaulino leninpaulino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

Although I don't fully understand the purpose of this PR, I noticed some minor improvements that would make the developer experience a bit smoother, especially for first-time contributors or people willing to understand or learn from this open-source project.

Most of these suggestions are related to the type hinting and documentation, which I think could help new devs saving a lot of time because they won't have to read more code to get an idea of how some methods/functions work.

I wasn't able to test if any of these suggestions will raise conflicts with the existent code, but invite everyone to start doing this whenever it is possible.

src/Core/Entity/FormRepository.php Outdated Show resolved Hide resolved
src/App/Repository/FormRepository.php Outdated Show resolved Hide resolved
src/App/Repository/PostRepository.php Outdated Show resolved Hide resolved
src/App/Repository/PostRepository.php Outdated Show resolved Hide resolved
src/App/Repository/PostRepository.php Outdated Show resolved Hide resolved
src/App/Repository/PostRepository.php Outdated Show resolved Hide resolved
tuxpiper and others added 2 commits April 6, 2020 19:51
@tuxpiper
Copy link
Member Author

tuxpiper commented Apr 6, 2020

I think this should be good for merging and QA now

@rowasc
Copy link
Contributor

rowasc commented Apr 8, 2020

@tuxpiper sorry I assumed we were already testing :( Yes let's push to steve-buscemi if we haven't and get Walter to do some QA so we can push this and the cache changes to prod this week.

Copy link
Contributor

@rowasc rowasc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets get this tested

@rowasc rowasc merged commit 02e371e into develop Apr 8, 2020
@rowasc rowasc deleted the geojson-optimization branch April 8, 2020 14:51
@rowasc
Copy link
Contributor

rowasc commented Apr 8, 2020

@Obadha2 can you test this ? (this should be the highest priority to test)

@AmTryingMyBest
Copy link
Contributor

@rowasc I found this one issue logged here
But after consulting with @tuxpiper agreed this wasn't a blocker.

@rowasc rowasc mentioned this pull request Apr 10, 2020
1 task
@rowasc rowasc mentioned this pull request Apr 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants