feat: add update method to register endpoint

[xav]

Description

Implement the update functionality in the register endpoint
Resolves #60

How Has This Been Tested?

Unit tests

Due Diligence

• Breaking change
• Requires a documentation update
• Requires a e2e/integration test update

[chris13524]

Would a HashSet be better?

Suggested change

	let mut tags = registration.tags;
	if let Some(remove_tags) = remove_tags {
	for r_tag in remove_tags.iter() {
	if let Some(index) = tags.iter().position(|tag| tag.eq(r_tag)) {
	tags.remove(index);
	}
	}
	}
	if let Some(append_tags) = append_tags {
	for a_tag in append_tags.iter() {
	if !tags.contains(a_tag) {
	tags.push(a_tag.clone());
	}
	}
	}
	let new_tags = registration.tags.into_iter().collect::<HashSet<_>>()
	.difference(&remove_tags.unwrap_or_else(|| HashSet::new())).collect::<HashSet<_>>()
	.union(&append_tags.unwrap_or_else(|| HashSet::new())).collect::<HashSet<_>>()
	.into_iter().collect::<Vec<_>>();

[xav]

This one doesn't compile. A form that actually compiles is this:

    let tags = registration
        .tags
        .into_iter()
        .collect::<HashSet<_>>()
        .difference(&remove_tags.unwrap_or_else(|| HashSet::new()))
        .map(|t| t.clone())
        .collect::<HashSet<_>>()
        .union(&append_tags.unwrap_or_else(|| HashSet::new()))
        .map(|t| t.clone())
        .collect::<HashSet<_>>()
        .into_iter()
        .collect::<Vec<_>>();

This is really verging on write-only code, and I'm not sure there are a lot of computational advantages to this form (especially given the expected size of the collections).

[chris13524]

My concern was more over readability/guaranteed set behavior than performance. Thought set operations would be easier to understand and easier to make sure the correct behavior is implemented.

[chris13524]

I think this is a slightly cleaned up version that compiles:

    let tags = registration
        .tags
        .into_iter()
        .collect::<HashSet<_>>()
        .difference(&remove_tags.unwrap_or_else(|| HashSet::new()))
        .cloned()
        .collect::<HashSet<_>>()
        .union(&append_tags.unwrap_or_else(|| HashSet::new()))
        .cloned()
        .collect::<Vec<_>>();

[chris13524]

Up to you. Just a suggestion

[xav]

Sure, and I understand that in some way it's more elegant and could be more efficient on bigger collections, but I don't think we'll ever get anything much bigger than 10 items, so I'd rather go with legibility 😀

[chris13524]

Again, performance wasn't my concern haha. Set operations are more legible IMO than loops and if statements, and they automatically avoid any edge cases we haven't thought of

[chris13524]

Suggested change

	for tag in remove_tags.iter() {
	if append_tags.contains(tag) {
	return Err(Error::InvalidUpdateRequest);
	}
	}
	if remove_tags.intersect(&append_tags).count() > 0 {
	return Err(Error::InvalidUpdateRequest);
	}

[xav]

Found a middle ground 😊

[xav]

I think the necessity to collect the iterators between each step feels kinda stupid tho, bad language design 😊

[chris13524]

Agree, what you did is much nicer!