Skip to content

Studio 59: encode/decode primary key before embedding in url and after parsing from url. - #303

Merged
foundling merged 5 commits into
stagefrom
STUDIO-59
Nov 7, 2023
Merged

Studio 59: encode/decode primary key before embedding in url and after parsing from url.#303
foundling merged 5 commits into
stagefrom
STUDIO-59

Conversation

@foundling

@foundling foundling commented Jul 5, 2023

Copy link
Copy Markdown
Contributor

The hash_id / primary key was getting passed unencoded into the dynamically constructed url that triggers the JSONEditor to render a record for editing. This PR encodes/decodes the value properly when embedding in and parsing it from the studio url to prevent parsing bugs.

…s a uriComponent when embedding it in a url and decode it after its been parsed as a url param.

@kriszyp kriszyp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

FYI, this is potentially getting to get more complicated in the future as we need to add support for multi-segment ids. So these are valid ids in the future (not sure if in 4.2 or later):

  • "simple string"
  • 42
  • ["multi", "part"]
  • ["could", "have a / or ? or number in it", 22]

In MQTT and REST the latter would be encoded/referenced by: table/could/have%20a%20%2F%20or%20%3F%20or%20number%20in%20it/22

navigate(`/o/${customer_id}/i/${compute_stack_id}/browse/${schema}/${table}/edit/${rowData[tableState.hashAttribute]}`, {
const encodedHash = encodeURIComponent(hashValue); // encode because the hashValue can contain url components

navigate(`/o/${customer_id}/i/${compute_stack_id}/browse/${schema}/${table}/edit/${encodedHash}`, {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I believe the schema and table names should also be encoded, as there are a number of allow characters that require encoding.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I now encode and decode those.

Currently, I get a front-end studio error telling me that only alphanumeric or underscores are allowed. Sounds like I should adjust the allowed characters as well as the message, but maybe that's a policy decision.

Is this the place to go for allowed characters in the schema name and table name? https://github.com/HarperDB/harperdb/blob/a46383d2e537e5e93c712a038e09ef19c9a77459/validation/common_validators.js#L5

If understand correctly, the regex (/^[\x20-\x2E|\x30-\x5F|\x61-\x7E]*$/) is specifying in hex 3 ranges of allowed ascii characters. Maybe I can just use that regex in studio and unpack the values into a more up-to-date and thorough client-side error message?

Another question: is there any plan to allow unicode in hash_value, schema or table names?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the regex (/^[\x20-\x2E|\x30-\x5F|\x61-\x7E]*$/) is specifying in hex 3 ranges of allowed ascii characters. Maybe I can just use that regex in studio and unpack the values into a more up-to-date and thorough client-side error message?

Yes, that should be correct, and I think that would be appropriate for studio to use as well.

Another question: is there any plan to allow unicode in hash_value, schema or table names?

Yes, we should absolutely do this. It is kind of not very inclusive to restrict to latin characters.

@foundling

foundling commented Jul 7, 2023

Copy link
Copy Markdown
Contributor Author
  • ["could", "have a / or ? or number in it", 22]

Thanks for the advisory note here. Am I ok to support this before the functionality is available?

@foundling
foundling requested a review from kriszyp July 7, 2023 15:09
@sonarqubecloud

sonarqubecloud Bot commented Jul 7, 2023

Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@kriszyp

kriszyp commented Jul 7, 2023

Copy link
Copy Markdown
Member

Thanks for the advisory note here. Am I ok to support this before the functionality is available?

Yes, I think that should be fine, like you said, it should be backwards compatible since nothing should have these ids right now.

@foundling
foundling force-pushed the stage branch 2 times, most recently from f3b6f0e to 8d7f915 Compare October 9, 2023 18:15
@sonarqubecloud

sonarqubecloud Bot commented Nov 7, 2023

Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@foundling
foundling merged commit d820f53 into stage Nov 7, 2023
@deliciousmonster
deliciousmonster deleted the STUDIO-59 branch January 4, 2024 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants