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

feat: Find Options - Update components API #552

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

jonas-martinez
Copy link
Collaborator

@jonas-martinez jonas-martinez commented Feb 22, 2024

About this PR

Closes #

Technical highlight/advice

How to test my changes

Checklist

  • I didn't over-scope my PR
  • My PR title matches the commit convention
  • I did not include breaking changes
  • I made my own code-review before requesting one

I included unit tests that cover my changes

  • 👍 yes
  • 🙅 no, because they aren't needed
  • 🙋 no, because I need help

I added/updated the documentation about my changes

  • 📜 README.md
  • 📕 docs/*.md
  • 📓 docs.lenra.io
  • 🙅 no documentation needed

@jonas-martinez jonas-martinez added the enhancement New feature or request label Feb 22, 2024
@jonas-martinez jonas-martinez self-assigned this Feb 22, 2024
@jonas-martinez
Copy link
Collaborator Author

@lenra-io/devs I might need help from someone that understands the query_server/view_uid/route_server/lenra_builder features.

I don't understand how I can add my options (skip & limit) params from my view find query. It seems that the work that was done on this only added support for the listeners part of this, this means that it does not work when using the find method in a view.

Can we plan a team meeting to discuss this subject ?

@jonas-martinez
Copy link
Collaborator Author

jonas-martinez commented Feb 29, 2024

@lenra-io/devs Finally I found a solution to make it work but I still have one last question about the QueryServer. I added an empty map %{} for each call to ViewServer.group_name() for the options, it works with that but I don't know what is the real purpose of this.

Does someone know what is the purpose of the Swarm.publish with the :data_changed event ?

Example: QueryServer : 440-441

group = ViewServer.group_name(env_id, coll, query_parsed, %{}, %{})
Swarm.publish(group, {:data_changed, new_data})

@taorepoara
Copy link
Member

I think that is made to notify the view servers that the data for a given request changed to rebuild the corresponding views

@taorepoara
Copy link
Member

You need to fill the PR description

@taorepoara
Copy link
Member

For this kind of feature you must add unit tests to check the find requests with and without options

@jonas-martinez
Copy link
Collaborator Author

@lenra-io/devs I might need some help from one of you.

I don't understand how to properly handle the data_changed event with the new "options" object.
In fact, the way the projections are currently handled is really weird to me. Let me explain :

When starting a QueryServer, we run a request to Mongo to get the initial data (not applying projections to the request). We get the full data and THEN we apply manually the projection with a Enum.map and Map.filter which seems weird, why not let mongo do it when running the request ??
This also means that the data is never updated if the projection changes. I do not see where is called the Mongo query outside of the initial_data method. At least this is what I understood.

If we want to keep the projection system as is, I will need some help to better understand the core principles of the queryServer and related code.

If we want to let Mongo do the work, a lot of refactoring will need to be done and the system will be MUCH easier.

{:ok, projection} <- Keyword.fetch(opts, :projection) do
{:ok, projection} <- Keyword.fetch(opts, :projection),
{:ok, options} <- Keyword.fetch(opts, :options),
{:ok, data} <- fetch_initial_data(env_id, coll, query_transformed, projection, options) do
Copy link
Member

Choose a reason for hiding this comment

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

The initial data should not be fetched with the projections and options since they both should be applied after.
The initial data must stay full to apply the projections and options.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok so this PR will need a lot more work to handle each mongo aggregators by hand.

We will start by just implementing the "skip" and "limit" aggregators and we will add more in the future.

Copy link
Member

Choose a reason for hiding this comment

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

Yes. You can create an Epic with the list of the aggregators to manage as we did for the operators

@@ -418,14 +427,14 @@ defmodule ApplicationRunner.Environment.QueryServer do
if projection_change?(projection_data, new_data, k) do
{k, v}
else
group = ViewServer.group_name(env_id, coll, query_parsed, k)
group = ViewServer.group_name(env_id, coll, query_parsed, k, %{})
Copy link
Member

Choose a reason for hiding this comment

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

Modification not needed:

Suggested change
group = ViewServer.group_name(env_id, coll, query_parsed, k, %{})
group = ViewServer.group_name(env_id, coll, query_parsed, k)

Swarm.publish(group, {:data_changed, projection_data(new_data, k)})
{k, projection_data(new_data, k)}
end
end)

# Notify ViewServer with no projection.
group = ViewServer.group_name(env_id, coll, query_parsed, %{})
group = ViewServer.group_name(env_id, coll, query_parsed, %{}, %{})
Copy link
Member

Choose a reason for hiding this comment

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

Same:

Suggested change
group = ViewServer.group_name(env_id, coll, query_parsed, %{}, %{})
group = ViewServer.group_name(env_id, coll, query_parsed, %{})

@@ -441,11 +450,11 @@ defmodule ApplicationRunner.Environment.QueryServer do
}
) do
Enum.each(Map.keys(projection_data), fn projection_key ->
group = ViewServer.group_name(env_id, old_coll, query_parsed, projection_key)
group = ViewServer.group_name(env_id, old_coll, query_parsed, projection_key, %{})
Copy link
Member

Choose a reason for hiding this comment

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

Same:

Suggested change
group = ViewServer.group_name(env_id, old_coll, query_parsed, projection_key, %{})
group = ViewServer.group_name(env_id, old_coll, query_parsed, projection_key)

Swarm.publish(group, {:coll_changed, new_coll})
end)

group = ViewServer.group_name(env_id, old_coll, query_parsed, %{})
group = ViewServer.group_name(env_id, old_coll, query_parsed, %{}, %{})
Copy link
Member

Choose a reason for hiding this comment

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

Same:

Suggested change
group = ViewServer.group_name(env_id, old_coll, query_parsed, %{}, %{})
group = ViewServer.group_name(env_id, old_coll, query_parsed, %{})

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

2 participants