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

RFC: Enabling GraphQL endpoint to support special characters in column names #1860

Open
vadeveka opened this issue Nov 3, 2023 · 6 comments
Assignees
Labels
rfc Request for comment

Comments

@vadeveka
Copy link
Contributor

vadeveka commented Nov 3, 2023

Problem statement

A table or a view in SQL may have special characters such as space for column names. If the developer does not provide explicit fieldmappings under the entity in config, then the field names are derived as is from column names. However special characters such as spaces are not supported in GraphQL names(acceptable characters per spec) and while DAB starts up fine, the GraphQL calls fail due to invalid schema. Specifying fieldmappings for applicable entities may be cumbersome process if numerous table names have special characters. Note that the issue does not impact REST endpoint/swagger as special characters for entity fields are escaped and accepted.

See related discussion on #1528, #692 and #1479.

There are several ways we can look to address the issue for GraphQL involved if no field mappings provided.

1. Throw error upfront

  • If GraphQL or GraphQL + REST is enabled in DAB, then during provisioning DAB will verify that column names are valid to be used as GraphQL field names.
  • If invalid, then throw fatal error during startup/config enablement and provide suggestion to use fieldmappings in config.
  • If GraphQL is not enabled, then this behavior does not apply

2. Introduce config option sanitize-fields-for-graphql standardize-fieldnames with boolean type defaulting to false.

This approach looks to sanitize the field names only when invalid and avoid impacting REST by default.

  • If GraphQL or GraphQL + REST is enabled in DAB, then during provisioning DAB will verify that column names are valid to be used as GraphQL field names based on below conditions.
  • If standardize-fieldnames= false and invalid name, then throw error upfront similar to previous approach indicating that valid field names could not be derived and to specify fieldmappings in config.
  • If standardize-fieldnames=true and invalid name, then sanitize the column names to field names using this styling convention (already being leveraged in DAB for query names for relations).
    • Given GraphQL names can only be in English chars, if the name sanitization results in no chars left, then throw error indicating that valid field names could not be derived and to use fieldmappings.
  • If standardize-fieldnames=true and valid name, no sanitization will occur even if the derived field name does not follow styling convention.
    • Alternately we can choose to always sanitize all field names per the convention even if column names are valid for GraphQL naming but not per the convention.
  • Note that the sanitized field names will be used for both GraphQL and REST endpoint for parity across both endpoints and to avoid introducing complexity with maintaining separate mappings
  • Since this option would impact both GraphQL and REST, we have several locations where we can introduce this option
     "runtime": {
     "standardize-fieldnames": true,
      "graphql": {
         "enabled": true
       },
       "rest": {
      ....
      } 
    

3. Sanitize column names to field names whenever invalid characters present with no config option

  • In this approach, DAB will verify that column names are valid to be used as GraphQL field names.
  • If invalid, then sanitize column names to field names using the previously stated convention
  • These sanitized field names will also be used for REST endpoint for parity. This will be breaking change but we can minimize the impact by applying this policy only when GraphQL endpoint is enabled while REST may or may not be enabled.

I propose we go with Option 2 to avoid breaking changes for customers who have REST enabled. The documentation can clarify the impact of enabling this option.

Looking forward to feedback and alternative proposals if any.

@vadeveka vadeveka added the rfc Request for comment label Nov 3, 2023
@vadeveka vadeveka self-assigned this Nov 3, 2023
@Aniruddh25
Copy link
Contributor

For option 2, do we plan to do the following? If yes, why do we need it if the name is already as per convention?

Alternately we can choose to always sanitize all field names per the convention even if column names are valid for GraphQL naming.

Also, if we will be applying this for both REST and GraphQL, the name for the config option should reflect that. Perhaps, just name as sanitize-fields

@vadeveka
Copy link
Contributor Author

vadeveka commented Nov 8, 2023

Option 2 originally seeks to apply the naming convention only if characters are invalid for GraphQL naming. This means that if the names are valid (e.g. _myType) but not necessarily following the convention (recommends camcelcase and no underscore), there would be no sanitization applied.

The alternate proposal was to always sanitize the field names if not following convention. I dont suggest we do this although some may prefer that sanitize-fields should always enforce naming convention for clarity.

sanitize-fields as the option name sounds fine to go with.

@seantleonard
Copy link
Contributor

seantleonard commented Nov 16, 2023

Is #654 covered within this scope?

@vadeveka
Copy link
Contributor Author

vadeveka commented Nov 21, 2023

Good point. If the names are non-English characters, that may cause an impact to name sanitization.
Added below proposal:

Given GraphQL names can only be in English chars, if the name sanitization results in no chars left, then throw error indicating that valid field names could not be derived and to use fieldmappings.

@seantleonard
Copy link
Contributor

The word sanitize doesn't make sense for REST. Because REST doesn't require field name changes and GraphQL does, I would argue for a property name using the word "standard" like standardize-fieldnames.

  • GraphQL: align field names with GraphQL specification to enable scheme building success.
  • REST: 'standardize' field names to align with names used for GraphQL.

@vadeveka
Copy link
Contributor Author

fair point - updated proposal to change config option name

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rfc Request for comment
Projects
None yet
Development

No branches or pull requests

3 participants