Skip to content
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

Implement HashMap support the same way as Vec (PR #1070) #1071

Open
rimutaka opened this issue May 31, 2022 · 4 comments
Open

Implement HashMap support the same way as Vec (PR #1070) #1071

rimutaka opened this issue May 31, 2022 · 4 comments
Assignees
Labels
enhancement Improvement of existing features or bugfix
Milestone

Comments

@rimutaka
Copy link

I have a struct where some members are Vec and some are HashSet. They both should convert to/from a GraphQL list.

pub struct Keywords {
    pub full_list: Vec<String>,
    pub unique_list: HashSet<String>,
}

There is a built-in support for Vec, but not for HashSet. I would think that adding HashSet support would make life easier in cases like this.

I copied the impl code for Vec to enable HashMap support in PR #1070. So far it worked fine.

  1. Is there a reason not to do it?
  2. Is there a better way of doing it?

If the idea is sound, would you consider that PR?
I will need to add input support, tests and see if docs need updating before it is ready. Just wanted to check with the maintainers (@tyranron , @ilslv, @LegNeato ?) if I'm on the right track here.

@rimutaka rimutaka added the enhancement Improvement of existing features or bugfix label May 31, 2022
@ilslv
Copy link
Member

ilslv commented May 31, 2022

@rimutaka as an output type HashSet should work just fine, but I'm not sure what the behaviour should be on input. Should HashSet just remove duplicates silently or error?

@tyranron
Copy link
Member

@rimutaka HashSet and GraphQL List have different sematics. The later is explicitly stated to be ordered:

GraphQL services must return an ordered list as the result of a list type.

I'm unsure what implications we will have. At first glance this seems to be OK for output, as with usual Vecs the user still can do any ordering. For input, though, the validation is happened before we resolve input value into HashSet, so the error reporting should be preserved, so seems to be OK too.


@ilslv

Should HashSet just remove duplicates silently or error?

I think error, as we do for arrays. If the user uses HashSet in schema, he wants to expose its invariants there. If he doesn't, nothing stops him from using Vec and then reduce it to HashSet.

@rimutaka
Copy link
Author

@ilslv , @tyranron , thanks for your prompt replies! You raised some good points.

  1. HashSet stores and outputs elements in random order. Not sure if this is a blocker. In the end, if the user chooses a HashSet over a Vec then the order is probably not important.

  2. serde quietly dedupes the input and HashMap::insert() returns bool, not Error, so the default seems to be to ignore the duplicates. Here is a simple example with multiple 3s:

println!("{:?}", serde_json::from_str::<HashSet<i32>>("[1,2,3,3,3]").unwrap());

prints {3, 2, 1}

It seems ignoring duplicates is the default with HashSets. I am not opposed to an error.

  1. I havn't got to the point of checking what HashSet looks like in the schema or trying it with GraphQL input.

Let me investigate a bit more before taking it any further.

@tyranron
Copy link
Member

@rimutaka

In the end, if the user chooses a HashSet over a Vec then the order is probably not important.

Yup, from the server side. I wonder if it may have some implications on a client side.

  1. I havn't got to the point of checking what HashSet looks like in the schema or trying it with GraphQL input.

Well, on the schema level HashSet will look like a regular GraphQL List for both input and output. Fully similar to how Vec looks like.

It seems ignoring duplicates is the default with HashSets. I am not opposed to an error.

I'm unsure about duplicates, despite serde_json does it. GraphQL differs from JSON in the way it allows to define custom semantics. Imagine we have a query ListLength(list: [Int]!): Int!, then the implementor may accidentally broke its semantics by choosing a HashSet as an argument which silently eats elements.

But It seems reasonable that library users may want HashSet eating duplicates because they don't bother.

It's hard to say for sure which case is more common. But I'd better go with strict defaults. We still may provide a transparent wrapper for a different behaviour:

#[derive(Clone, Debug, Deref, DerefMut, From, Into, RefCast)]
#[transparent]
struct DedupHashSet<T>(HashSet<T>);

and using it

#[derive(GraphQLInputObject)]
pub struct Keywords {
    pub full_list: Vec<String>,
    pub unique_list: juniper::types::DedupHashSet<String>,
}

will allow to use .unique_list field almost the same as it was HashSet.

@tyranron tyranron added this to the 0.16.0 milestone Jun 1, 2022
@tyranron tyranron modified the milestones: 0.16.0, 0.17.0 Oct 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement of existing features or bugfix
Projects
None yet
Development

No branches or pull requests

3 participants