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

CSV-5225: ignore nested documents that exceeds nested_objects_limit #2025

Merged
merged 4 commits into from
Sep 9, 2024

Conversation

melsawy
Copy link
Contributor

@melsawy melsawy commented Sep 8, 2024

Description

Ignore nested documents that exceeds nested_objects_limit which by default 10.000 .
I limit this condition to tags and requests nested objects.

References: CSV-5225

How has this been tested?

Implemented automated tests.

Things to pay attention to during code review

Please describe parts of the change that require extra attention during code review, for example:

  • File FFFF, line LL: This refactoring does this and this. Is it consistent with how it’s implemented elsewhere?
  • Etc.

Checklist

  • I have performed a self-review of my own code
  • I have added unit and feature tests, if the PR implements a new feature or otherwise would benefit from additional testing
  • I have added regression tests, if the PR fixes a bug
  • I have added logging, exception reporting, and custom tracing with any additional information required for debugging
  • I considered secure coding practices when writing this code. Any security concerns are noted above.
  • I have commented my code in hard-to-understand areas, if any
  • I have made needed changes to the README
  • My changes generate no new warnings
  • If I added a third party module, I included a rationale for doing so and followed our current guidelines

@melsawy melsawy marked this pull request as ready for review September 8, 2024 19:00
Copy link
Contributor

@caiosba caiosba left a comment

Choose a reason for hiding this comment

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

Thanks Sawy for making it more generic for any nested field! Just requested a few minor changes.

Comment on lines 315 to 320
# pm_tt2 = pm.annotations('task').select{|t| t.team_task_id == tt2.id}.last
# pm_tt2.response = { annotation_type: 'task_response_single_choice', set_fields: { response_single_choice: 'ans_aa' }.to_json }.to_json
# pm_tt2.save!
# pm_tt3 = pm.annotations('task').select{|t| t.team_task_id == tt3.id}.last
# pm_tt3.response = { annotation_type: 'task_response_single_choice', set_fields: { response_single_choice: 'ans_aaa' }.to_json }.to_json
# pm_tt3.save!
Copy link
Contributor

Choose a reason for hiding this comment

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

Why these are commented? If not relevant, can't you just delete them?

# pm_tt3.save!
sleep 2
es = $repository.find(pm.get_es_doc_id)
pp es['task_responses']
Copy link
Contributor

Choose a reason for hiding this comment

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

Please delete this debugging line if you don't need it.

pm = create_project_media team: team
stub_configs({ 'nested_objects_limit' => 2 }) do
# answer single choice
puts "Start to answer"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please delete this debugging line if you don't need it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK will do

@@ -82,6 +82,7 @@ def get_fresh_value(data)

def add_update_nested_obj(options)
return if self.disable_es_callbacks || RequestStore.store[:disable_es_callbacks]
return if options[:op] == 'create' && self.respond_to?(:hit_nested_objects_limit?) && self.hit_nested_objects_limit?
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Only happens when the operation is "create"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes as create add a new object to nested_object

@melsawy melsawy requested a review from caiosba September 8, 2024 19:28
Copy link
Contributor

@caiosba caiosba left a comment

Choose a reason for hiding this comment

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

Thanks Sawy, good to go once the test is fixed.

Comment on lines 322 to 323
# TODO: fix the test
# assert_equal 2, task_responses.size
Copy link
Contributor

@caiosba caiosba Sep 8, 2024

Choose a reason for hiding this comment

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

Good to go once this test is fixed.

@melsawy melsawy merged commit c955714 into develop Sep 9, 2024
10 checks passed
@melsawy melsawy deleted the fix/CV2-5225-elasticsearch-nested-objects-limit branch September 9, 2024 08:23
melsawy added a commit that referenced this pull request Sep 9, 2024
…2025)

* CSV-5225: ignore nested documents that exceeds nested_objects_limit

* CV2-5225: add tests

* CV2-5225: cleanup

* CV2-5225: apply nested_objects_limit on tags and requests
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.

2 participants