Question regarding tables::node::Table::update

[sophie-cluml]

Hello,
I have a question regarding this line : https://github.com/petabi/review-database/blob/main/src/tables/node.rs#L241

I think tables::node::Table::update not only updates the node table itself, but also updates agent table in pretty much cascading manner. And I think this code (https://github.com/petabi/review-database/blob/main/src/tables/node.rs#L238-L248) aims to update the agent table only when Agent needs an update. If so, I am afraid that .filter(|(o, n)| **o == **n) might possibly be .filter(|(o, n)| **o != **n).

But it is possible that I misunderstood the intention of this update method. If current implementation is intended, then I think review-web will need to iterate over Node::agents vector and call Agent::update, to make sure that agents that already existed in the Node::agents list also get updated, along with calling Node::update. Could you advise me on this please?

[sophie-cluml]

I personally tried this ad-hoc test, which was written according to my expectations, and it failed, which was the starting point for this question.

    #[test]
    fn update_agents_drafts_only() {
        let store: Arc<Store> = setup_store();
        let kinds = vec![AgentKind::Reconverge, AgentKind::Piglet, AgentKind::Hog];
        let configs1: Vec<_> = create_configs(&kinds);
        let configs2 = vec![None, None, None];

        let profile = Profile::default();

        let agents = create_agents(123, &kinds, &configs1, &configs2);

        let mut node = create_node(
            456,
            "test",
            None,
            None,
            Some(profile.clone()),
            agents.clone(),
        );

        let mut node_table = store.node_map();

        let res = node_table.put(node.clone());
        assert!(res.is_ok());

        // update node id to the actual id in database.
        node.id = res.unwrap();
        node.agents.iter_mut().for_each(|a| a.node = node.id);

        let id = node.id;

        let old = node.clone().into();
        let mut update = node.clone();
        update.agents = update.agents.into_iter().map(|mut a| {
            // the `draft` field of each agent
            a.draft = Some("my_key=10".to_string().try_into().unwrap());
            a
        }).collect();
        let update = update.into();
        assert!(node_table.update(id, &old, &update).is_ok());

        let updated = node_table.get_by_id(id).unwrap();
        assert!(updated.is_some());
        let (updated, invalid) = updated.unwrap();
        assert!(invalid.is_empty());

        assert_eq!(updated.agents, update.agents);  //  expected success but failed
    }

[minshao]

You are right about Node::update, it should update the corresponding agent in a cascading manner.

As to why the unit test fail, I will get back to you next Monday, with a fix if it's a bug.

Thank you for pointing it out and really appreciate the unit test. It's very helpful!

[sophie-cluml]

Thank you, I appreciate your help. 🙇

[minshao]

See #332
🙇

[sehkone]

@minshao
Could you answer the above question? (I just mentioned you because @sophie-cluml didn't.)