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

Fix Mapper Insert method #75

Merged
merged 1 commit into from
Mar 13, 2019
Merged

Fix Mapper Insert method #75

merged 1 commit into from
Mar 13, 2019

Conversation

micbar
Copy link
Contributor

@micbar micbar commented Mar 12, 2019

Bug

The Status Mappers insert method does not return the fileid of the entity.

Problem

This sets all scanned files on occ search:index --all to Status "N" (New)

Solution

Fix the mappers insert method

@micbar micbar self-assigned this Mar 12, 2019
@micbar micbar requested review from butonic and patrickjahns March 12, 2019 16:09
@micbar micbar added this to the development milestone Mar 12, 2019
@micbar micbar mentioned this pull request Mar 12, 2019
13 tasks
@codecov
Copy link

codecov bot commented Mar 12, 2019

Codecov Report

Merging #75 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##             master     #75      +/-   ##
===========================================
+ Coverage      2.96%   2.96%   +<.01%     
  Complexity      225     225              
===========================================
  Files            22      22              
  Lines          1012    1011       -1     
===========================================
  Hits             30      30              
+ Misses          982     981       -1
Impacted Files Coverage Δ Complexity Δ
db/statusmapper.php 0% <ø> (ø) 35 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5033c9e...74a6985. Read the comment docs.

@patrickjahns
Copy link
Contributor

What effect does this behavior have on the normal usage?

@micbar
Copy link
Contributor Author

micbar commented Mar 13, 2019

Before the Fix

  1. occ search:reset & occ search:index --all
  2. All files in the index get Status "N" New due to error in Mapper Insert method

The Status gets created in the DB here https://github.com/owncloud/search_elastic/blob/master/db/statusmapper.php#L289
The fileId is 0 in the returned status object https://github.com/owncloud/search_elastic/blob/master/lib/searchelasticservice.php#L237 and all further changes on the Status in the different branches of the indexNodes() method are run with fileID = 0 That causes the Status to remain unchanged, even if it is successfully indexed, vanished or has an error.

Bad Side Effect

If the first Background Job for indexing a new file or updating content or metatada is run all status entries in the oc_search_elastic_status table are corrected which would take some time on a big Instance with a lot of files which has been indexed by seach:index --all due to a migration.

Effect of the fix on the normal usage:

  1. New files will get Status "N"
  2. After that, they will be processed normally and the Status will change by the Background Job.

@patrickjahns @butonic

Copy link
Member

@butonic butonic left a comment

Choose a reason for hiding this comment

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

makes sense. we saw when debugging this that the db always returned 0 for the getLastInsertId() call. @micbar what db was running, btw?

I assume that since the entitiy comes with an id no autoincrement is triggered, hence no id is generated.

The code that creates the id for the entitiy is in https://github.com/owncloud/search_elastic/blob/master/db/statusmapper.php#L279-L291

We always pass in the id because we get it from the filecache first.

If anything we could think about making the fileid column a foreign key column ... but that is a different issue.

Copy link
Contributor

@patrickjahns patrickjahns left a comment

Choose a reason for hiding this comment

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

LGTM

@micbar
Copy link
Contributor Author

micbar commented Mar 13, 2019

@butonic MariaDB

@micbar micbar merged commit 4e0096c into master Mar 13, 2019
@delete-merged-branch delete-merged-branch bot deleted the fix/mapper branch March 13, 2019 08:57
@micbar micbar mentioned this pull request Mar 14, 2019
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.

3 participants