Skip to content

Allow special characters in an enum #12200

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 3 commits into from
Mar 8, 2022

Conversation

petersg83
Copy link
Contributor

@petersg83 petersg83 commented Jan 14, 2022

Fix #11703

Allow special characters in an enum.

REST: Nothing special
GraphQL: The enum input uses the normalized format (ex: if your enum value is démocratie then your GraphQL input should be democratie). The enum output will correctly be démocratie).

# request
mutation {
  createCountry(data: { enumField: democratie }) {
    data {
      id
      attributes {
        enumField
      }
    }
  }
}
# response
{
  "data": {
    "createCountry": {
      "data": {
        "id": "18",
        "attributes": {
          "enumField": "démocratie"
        }
      }
    }
  }
}

EDIT: after discussing it, the output will finally also be democratie

@petersg83 petersg83 added issue: enhancement Issue suggesting an enhancement to an existing feature source: core:content-type-builder Source is core/content-type-builder package labels Jan 14, 2022
@petersg83 petersg83 added this to the 4.1.0 milestone Jan 14, 2022
@codecov
Copy link

codecov bot commented Jan 14, 2022

Codecov Report

Merging #12200 (96e41eb) into master (3700b15) will increase coverage by 0.01%.
The diff coverage is 80.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12200      +/-   ##
==========================================
+ Coverage   47.61%   47.62%   +0.01%     
==========================================
  Files         228      228              
  Lines        8515     8517       +2     
  Branches     1901     1901              
==========================================
+ Hits         4054     4056       +2     
  Misses       3666     3666              
  Partials      795      795              
Flag Coverage Δ
front ?
unit 47.62% <80.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/core/utils/lib/index.js 100.00% <ø> (ø)
...pe-builder/server/controllers/validation/common.js 66.66% <50.00%> (-0.84%) ⬇️
packages/core/utils/lib/string-formatting.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3700b15...96e41eb. Read the comment docs.

// Scalars
else if (isStrapiScalar(attribute)) {
if (isStrapiScalar(attribute)) {
Copy link
Contributor

@iicdii iicdii Jan 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there the specific reason you remove codes in this file? It reverts the fix in #12005

master

before

this PR

after

Copy link
Contributor Author

@petersg83 petersg83 Jan 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good eye :)
The input stays as an enum but the output is put as a string so special characters could be returned. I put an example in the PR description. Do you see a problem with that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, I just found that special characters are not supported in GraphQL specs..
http://spec.graphql.org/October2021/#note-46408

Names in GraphQL are limited to the Latin ASCII subset of SourceCharacter in order to support interoperation with as many other systems as possible.

I think there will be more advantages of using the string type than using the enum type :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, thank you :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was just about to ask if this is a good idea since we just had an issue that GraphQL was returning strings for enums.... 🤔 It would be nice to put this issue to bed honestly

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iicdii fixed this in #12005

Copy link
Contributor Author

@petersg83 petersg83 Jan 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iicdii considering that this PR only changes the output schema and not the input, do you think it would create problems for the usecases of people listed above? I'm not familiar with typescript. @Convly what do you think ?

Copy link
Contributor

@iicdii iicdii Jan 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's trade-off. It lose some benefit of autogenerating api documentation If you force to enum to be a string. Otherwise, you can't use special characters because of GraphQL spec.
I think the way to satisfy both sides is to give the user an option to use special characters or not. If user checked to use special chracters, enum types are shown as string. Otherwise, enum types are shown as enum, and the validation should be applied not to allow special characters.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, it seems a reasonable proposition to put a config option for that. What do you think @alexandrebodin @Convly and @derrickmehaffy ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me, though I would advise by default we shouldn't allow it.

@petersg83 petersg83 force-pushed the content-39/allow-more-characters-in-enum branch from 261c5af to 2730f62 Compare January 21, 2022 14:33
@petersg83
Copy link
Contributor Author

After discussing internally, the field will be a graphql enum for both input and output. The pain point to not have special characters in the enum in the response will be handled in the future with a new type that would look like enum with a key/value system.

@derrickmehaffy
Copy link
Member

After discussing internally, the field will be a graphql enum for both input and output. The pain point to not have special characters in the enum in the response will be handled in the future with a new type that would look like enum with a key/value system.

Indeed, I think this the better way going forward, enum are a special format type that's designed to be read by multiple types of languages: https://en.wikipedia.org/wiki/Enumerated_type

There are usually a lot of restrictions on the format of enumerations depending on the language but restricting chars is a common one.

@petersg83 petersg83 requested review from gu-stav and Convly February 14, 2022 10:57
@alexandrebodin alexandrebodin modified the milestones: 4.1.0, 4.1.1 Feb 17, 2022
@petersg83 petersg83 added flag: don't merge This PR should not be merged at the moment and removed flag: don't merge This PR should not be merged at the moment labels Feb 21, 2022
@alexandrebodin alexandrebodin self-assigned this Feb 23, 2022
@alexandrebodin alexandrebodin modified the milestones: 4.1.1, 4.1.2 Feb 24, 2022
@alexandrebodin
Copy link
Member

I have been testing the PR but I get an issue with graphQL now I think.

When doing a query I get the normalized value in the response but I can only filter with the non-normalized value. I was expecting the 2 to be the same?

@alexandrebodin alexandrebodin added pr: enhancement This PR adds or updates some part of the codebase or features and removed issue: enhancement Issue suggesting an enhancement to an existing feature labels Mar 2, 2022
@alexandrebodin alexandrebodin modified the milestones: 4.1.2, 4.1.3 Mar 2, 2022
@petersg83
Copy link
Contributor Author

It's a good question if it is an issue or not. By using the normalized values in filters, you prevent the user to use some operators (like contains and partially gt, lt...). But I understand why we would want that.
capture_d___e__cran_2022-03-03_a___16 48 19

In any case it is already the actual behavior, so we can maybe handle this in another PR if needed?

@petersg83 petersg83 force-pushed the content-39/allow-more-characters-in-enum branch from 6ac247a to 96e41eb Compare March 3, 2022 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: enhancement This PR adds or updates some part of the codebase or features source: core:content-type-builder Source is core/content-type-builder package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Key-value pairs in enumeration field
6 participants