Skip to content

Allow cheap conversion intoScalarValue for custom string types #1324

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

Merged
merged 10 commits into from
May 30, 2025

Conversation

tyranron
Copy link
Member

@tyranron tyranron commented May 28, 2025

Part of #819
Revealed from #1247 (comment)

Synopsis

an idea: Could we somehow make From<ArcStr> a supertrait of ScalarValue? Then one could make a custom scalar value that doesn't need to clone string buffers that are already ArcStr. Think introspection queries, etc.

(Ideally From<&ArcStr> (which doesn't require cloning up front), but that's not possible without making ScalarValue lifetime generic. Another idea is to add a method fn from_arcstr(&ArcStr) -> Self to trait ScalarValue itself.)

edit: fn from_arcstr(&ArcStr) could be a method with a default implementation that just delegates to From<String>

Indeed, if an ArcStr is used (like in SchemaType types), and some custom ScalarValue is involved, which uses ArcStr internally, we want to operate on it without intermediate conversions into/from String.

Solution

To allow other custom string types as well, instead of just fn from_arcstr(&ArcStr) -> Self method, we introduce a fn from_displayable<Str: fmt::Display + Any + ?Sized>(s: &Str) -> Self method defaulting to s.to_string().into().

However, an author of a custom ScalarValue could redefine it to convert efficiently:

fn from_displayable<Str: fmt::Display + Any + ?Sized>(s: &Str) -> Self {
    use juniper::AnyExt as _;

    if let Some(arc_str) = s.downcast_ref::<ArcStr>() {
        Self::ArcStr(arc_str.clone())
    } else {
        s.to_string().into()
    }
}

Checklist

  • Documentation is updated
  • Examples are provided
  • Tests are added
  • CHANGELOG is updated

@tyranron tyranron added this to the 0.17.0 milestone May 28, 2025
@tyranron tyranron self-assigned this May 28, 2025
@tyranron tyranron added enhancement Improvement of existing features or bugfix k::api Related to API (application interface) k::performance Related to performance k::refactor Refactoring, technical debt elimination and other improvements of existing code base feature New feature or request labels May 28, 2025
@tyranron tyranron changed the title Allow cheap conversions info/from ScalarValue for custom string types Allow cheap conversions infoScalarValue for custom string types May 30, 2025
@tyranron tyranron changed the title Allow cheap conversions infoScalarValue for custom string types Allow cheap conversion intoScalarValue for custom string types May 30, 2025
@tyranron
Copy link
Member Author

tyranron commented May 30, 2025

ack @audunhalland

I went a little bit further with your idea, to not restrict only to ArcStr type, but allow arbitrary ones.

The second counterpart of this, allowing cheap conversion from InputValue into custom string type, would be done in a subsequent PR.

@tyranron tyranron marked this pull request as ready for review May 30, 2025 14:04
@audunhalland
Copy link
Contributor

@tyranron I like it, it's a nice improvement.

@tyranron tyranron enabled auto-merge (squash) May 30, 2025 15:21
@tyranron tyranron merged commit b9e94a6 into master May 30, 2025
180 checks passed
@tyranron tyranron deleted the arcstr-for-scalar-value branch May 30, 2025 15:22
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 feature New feature or request k::api Related to API (application interface) k::performance Related to performance k::refactor Refactoring, technical debt elimination and other improvements of existing code base
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants