Fixes #24723: Tree group is slow to load up because it contains the list of nodes in the tree #5621
Conversation
|
PR updated with a new commit |
| .withFieldComputed( | ||
| _.targetInfos, | ||
| _.targetInfos.collect { | ||
| case t @ FullRuleTargetInfo(_: FullOtherTarget, _, _, _, _) => t.toTargetInfo.transformInto[JRRuleTargetInfo] |
There was a problem hiding this comment.
FullOtherTarget because group targets are already in groups fields
| object JRGroupCategoryInfo { | ||
| final case class JRGroupInfo( | ||
| id: NodeGroupId, | ||
| @jsonField("displayName") name: String, |
There was a problem hiding this comment.
for this one and other: why not using the json name directly ? Is it an choice given other constraints ?
There was a problem hiding this comment.
(I imagine with chimney, but if so: is there a reason to chose that way ?
There was a problem hiding this comment.
It avoid a boilerplate for the Transformer definition in chimney : we can derive the field transformation if the name is the same as the source datatype. If we have the displayName field, we would have to write withFieldRenamed(_.displayName, _.name) (it is even more verbose if the transformer could be derive-d directly)
There was a problem hiding this comment.
but then, you wrote @jsonField("displayName")̀ :) OK, there's less chars in that latter option. And perhaps it's a bit clearer.
| override def dataContainer: Option[String] = Some("groupsinternal") | ||
| } | ||
|
|
||
| object GroupInternalApi extends Enum[GroupInternalApi] with ApiModuleProvider[GroupInternalApi] { |
There was a problem hiding this comment.
I think that for API used for a given (set of page / elm app), we should try to get a nomenclature based on the using app nomenclature.
Plus, we are now officially far far far away from REST and in full RPC land :)
There was a problem hiding this comment.
(I think it's ok for that one, and since it's an internal API we can change it whenever we want, but it is something to consider cc @RaphaelGauthier @VinceMacBuche )
|
This PR is not mergeable to upper versions. |
|
OK, squash merging this PR |
…ist of nodes in the tree
amousset
force-pushed
the
bug_24723/tree_group_is_slow_to_load_up_because_it_contains_the_list_of_nodes_in_the_tree
branch
from
d13daeb
to
253f1d3
Compare
https://issues.rudder.io/issues/24723
Adding an internal groups API and using it instead of the public one.
All the JSON datatype is made with zio-json and chimney, and it is almost the same model as a full group category, because all the information is needed in the Elm app except the node ids.
Also added API tests for that (FYI the tests for the public groups API does not have one for the tree endpoint)