-
-
Notifications
You must be signed in to change notification settings - Fork 0
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
base: main
Are you sure you want to change the base?
Conversation
@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 ? |
@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}) |
I think that is made to notify the view servers that the data for a given request changed to rebuild the corresponding views |
You need to fill the PR description |
For this kind of feature you must add unit tests to check the find requests with and without options |
@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. 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 ?? 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. |
…to update-component-api-options
{: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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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, %{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modification not needed:
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, %{}, %{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same:
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, %{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same:
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, %{}, %{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same:
group = ViewServer.group_name(env_id, old_coll, query_parsed, %{}, %{}) | |
group = ViewServer.group_name(env_id, old_coll, query_parsed, %{}) |
About this PR
Closes #
Technical highlight/advice
How to test my changes
Checklist
I included unit tests that cover my changes
I added/updated the documentation about my changes