Optional ID refactor: part 3

[figueroa1395]

Follow-up PR after #828. Closes #806

[figueroa1395]

The remaining problem comes from the fact that assert(static_cast<ptrdiff_t>(sequence_idx.size()) == std::distance(begin, end)); in update_component within main_model_impl.hpp doesn't hold anymore for permanent updates with mixed optional ids and invalid ids. I think this can be traced to the new implementation of get_all_sequence_idx_map, as the sequence size is not the same for every case, as it was before. Hence, a suggestion that doesn't involve changing the whole update_component flow would be welcome.

Note: Only the API tests regarding what's mentioned above are failing. Every other one, including on the Python side, passes just fine.

[mgovers]

The remaining problem comes from the fact that assert(static_cast<ptrdiff_t>(sequence_idx.size()) == std::distance(begin, end)); in update_component within main_model_impl.hpp doesn't hold anymore for permanent updates with mixed optional ids and invalid ids. I think this can be traced to the new implementation of get_all_sequence_idx_map, as the sequence size is not the same for every case, as it was before. Hence, a suggestion that doesn't involve changing the whole update_component flow would be welcome.

Note: Only the API tests regarding what's mentioned above are failing. Every other one, including on the Python side, passes just fine.

how doesn't it hold up anymore? is that because you have 2 steps now; one involving the globally cached components, and another involving the locally cached ones?

if so, you can maybe go with one (or a combination) of the following solutions:

1. something along the lines of for every component type that is cached: the assertion shall hold
2. only call the concerning function if sequence_idx.size() > 0
3. add fallback to the concerning function if sequence_idx.empty(). I think this was the logic before the original optional ID implementation
4. split the function up into 2 logical steps
5. if none of these are right, feel free to remove the assert if there's a good reason to remove it. it was just a sanity check; nothing more. nothing will break when you remove an assert (the only thing that can happen is that potential future bugs will be slightly harder to detect)

[Jerry-Jinfeng-Guo]

Nothing major left, only minor comments like above.

[mgovers]

no other comments from my end

[Jerry-Jinfeng-Guo]

Quality Gate passed

Issues 2 New issues 0 Accepted issues

Measures 0 Security Hotspots 96.8% Coverage on New Code 0.0% Duplication on New Code

See analysis details on SonarQube Cloud

The two issues are actually quite trivial to fix, could you patch them? @figueroa1395

[figueroa1395]

Quality Gate passed

Issues 2 New issues 0 Accepted issues
Measures 0 Security Hotspots 96.8% Coverage on New Code 0.0% Duplication on New Code
See analysis details on SonarQube Cloud

The two issues are actually quite trivial to fix, could you patch them? @figueroa1395

I had completely missed them. Thanks for the reminder. They have been addressed now.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
96.8% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud