You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/01/14 20:33:30 UTC

[GitHub] [superset] corbinrobb opened a new pull request #18056: fix(CRUD/listviews): [WIP] errors with rison and search strings using special characters

corbinrobb opened a new pull request #18056:
URL: https://github.com/apache/superset/pull/18056


   ### SUMMARY
   
   ### Short description:
   
   Fixes an issue with rison caused by differences of encoding/decoding between front end and back end dependencies.
   
   Also, fixes some issues caused by sending and/or retrieving URLs without cleaning/encoding potential URL breaking syntax added by the user.
   
   ---
   ### Detailed Description:
   
   This mostly comes down to three problems:
   - The JavaScript rison encodes/decodes in a slightly different way than the Python rison does
   - rison's encode method doesn't URL encode by default so URL queries can be broken by the user's input
   - When we retrieve a query string appended to a route (Refresh the page during a search), it gets read as a URL. So certain characters can break the URL and cause rison to try to read incomplete or improperly encoded/decoded strings.
   
   ### Rison encode/decode:
   This might seem complicated at first but it really comes down to a difference in a single Regular Expression (regex).
   
   To put it very simply, rison is a way to make a string from an object. It aims to make this string as compact, human-readable, and URL-friendly as possible. Read more about it [here](https://github.com/Nanonid/rison).
   
   It will take this:
   ```javascript
   { filters: [{ value: 'string' }, { value: 100 }] }
   ```
   and turn it into this:
   ```javascript
   (filters:!((value:string),(value:100)))
   ```
   
   - Both the JS rison and Python rison **can** encode any object thrown at it
   - Both the JS rison and Python rison **can** decode any string that _they_ encoded
   - Both the JS rison and Python rison **can't** decode all strings that the _other_ has encoded
   
   So what is happening?
   
   The answer comes down to the regex that I mentioned. 
   
   Both of them have a function that builds a string of characters to be used in the regex that decides whether a character can be a valid "id". An id is any value being used as a rison value. (filters, value, 'string', and 100 are all ids in the previous examples)
   
   Rison has reserved values that can't be used as ids but a "string" value that is passed can have _any_ characters inside of it as long as it has 'single quotes' around it. Rison encode will provide these 'single quotes' around any value that contains a character that it has reserved or restricted for its syntax.  This helps rison know a value is a string and thus parse the reserved characters correctly.
   
   The regex expression is what decides this and will fail to decode the string if it determines that single quotes weren't added correctly because it sees it as invalid syntax.
   
   So why is the regex different?
   
   I don't know the motivation behind this but as I mentioned, there is a loop that builds the string in the JS rison and the Python rison. Both of these loops build the same string.
   
   But for some reason, the JS rison overwrites this string with a different hard typed one immediately after it is made.
   
   JS:
   ```javascript
   (function () {
       var l = [];
       for (var hi = 0; hi < 16; hi++) {
           for (var lo = 0; lo < 16; lo++) {
               if (hi+lo === 0) continue;
               var c = String.fromCharCode(hi*16 + lo);
               if (! /\w|[-_.\/~]/.test(c))
                   l.push('\\u00' + hi.toString(16) + lo.toString(16));
           }
       }
       rison.not_idchar = l.join('');  // <<--- Where it is added
   })();
   //rison.not_idchar  = " \t\r\n\"<>[]{}'!=:(),*@$;&";
   rison.not_idchar  = " '!:(),*@$"; // <<--- Where it is reassigned
   ```
   
   Python:
   ```python
   NOT_IDCHAR = ''.join([c for c in (chr(i) for i in range(127))
                         if not (c.isalnum()
                                 or c in IDCHAR_PUNCTUATION)])
   
   # ^^^ Good ol python code not doing anything weird ^^^
   ```
   
   So all we need to do is change that id_char variable on the javascript rison and update it's regex. Then we have the javascript rison encoding the same way that the python rison is expecting it to be. 
   
   This fixes the issues that cause a 400 error when searching for these symbols by themselves 
   ```
   & # ? ^ { } [ ] | " = + `
   ```
   
   **[ MORE INFORMATION ABOUT THE FIX COMING SOON ]**
   
   ---
   
   ### Rison encode being in the URL friend(ly) zone
   
   Super simple here. Rison encode is URL friendly and is not perfect when it comes to URLs and rison knows this. So it provided a way to safely send URLs in the form of a method called `encode_uri`. Using this is not always necessary but it is useful when you are unsure about the data that is being provided in your string like when user input is involved. [Here](https://github.com/Nanonid/rison#interaction-with-uri--encoding) is where it is talked about in the README.
   
   So I switched to encode_uri for the API calls that are building strings that may have user input.
   
   This fixes the Fatal Errors that are returned from the backend when a search string includes & or # or includes a percent encoded value like a single quote (%27). This also prevents the user from being able to search using percent encoded values. For example, if you currently search %27%26%27 ('&') in any of the listviews it will actually return the results that include an ampersand.
   
   ### URL encoding for QueryParams
   Another simple explaination here. The list views use QueryParams to put the query string in the browser URL for search/filter persistence. This is done so you can do things like refresh the page or press the forward and back button without it completely removing the filters, sort orders, etc that you set. At least that's what I believe it is being used for.
   
   Looks like this:
   ```
   chart/list/?filters=(slice_name:hello)&pageIndex=0&sortColumn=changed_on_delta_humanized&sortOrder=desc&viewMode=card
   ```
   
   This part of it is rison:
   ```
   (slice_name:hello)
   ```
   
   QueryParams allows you to set custom encoding/decoding for certain Params but it will still always read from the params as a URL before it decodes. So this means that non percent encoded symbols that can break URLs like &, #, and unexpected percent encoded strings, will cause issues. 
   
   This is what is causing the current page breaking Fatal Error that happens on the list views when searching for an & by itself.
   
   This is the same fatal error that was happening on the backend but this time it is on the front end.
   
   I opted to not use the rison.encode_uri method for this because it doesn't keep the string that clean and will percent encode a little too much if certain characters are passed to it. I felt that since the user can see this query in their browser that the best approach would be to keep that minimal. 
   
   So the fix here is to encode only the characters that cause problems by using some String.replace() methods after using rison.encode()
   
   ```javascript
             rison.encode(data)
             .replace(/%/g, '%25')
             .replace(/&/g, '%26')
             .replace(/\+/g, '%2B')
             .replace(/#/g, '%23'),
   ```
   
   
   ---
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   <!--- Skip this if not applicable -->
   **[ COMING SOON ]**
   
   ### TESTING INSTRUCTIONS
   <!--- Required! What steps can be taken to manually verify the changes? -->
   **[ ALSO COMING SOON ]**
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] Has associated issue:
   - [ ] Required feature flags:
   - [ ] Changes UI
   - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
     - [ ] Migration is atomic, supports rollback & is backwards-compatible
     - [ ] Confirm DB migration upgrade and downgrade tested
     - [ ] Runtime estimates and downtime expectations provided
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] corbinrobb commented on pull request #18056: fix(CRUD/listviews): [WIP] errors with rison and search strings using special characters

Posted by GitBox <gi...@apache.org>.
corbinrobb commented on pull request #18056:
URL: https://github.com/apache/superset/pull/18056#issuecomment-1028367045


   Hey @pkdotson thanks for taking a look. I had to think about it a lot but I believe I may have a good idea of how I can do some unit tests on this. I will try to implement it and get it pushed up soon! I am going to try to add an end to end test for it as well. I have only a little bit of experience writing cypress tests so I am not sure how long that will take to add


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] betodealmeida commented on a change in pull request #18056: fix(CRUD/listviews): Errors with rison and search strings using special characters

Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #18056:
URL: https://github.com/apache/superset/pull/18056#discussion_r806390564



##########
File path: superset-frontend/src/views/CRUD/utils.test.tsx
##########
@@ -171,3 +172,21 @@ test('does not ask for password when the import type is wrong', () => {
   };
   expect(hasTerminalValidation(error.errors)).toBe(true);
 });
+
+test('successfully modified rison to encode correctly', () => {
+  const problemCharacters = '& # ? ^ { } [ ] | " = + `';
+
+  const testObject = problemCharacters.split(' ').reduce((a, c) => {
+    // eslint-disable-next-line no-param-reassign
+    a[c] = c;
+    return a;
+  }, {});
+
+  const actualEncoding = rison.encode(testObject);
+
+  const expectedEncoding =
+    "('\"':'\"','#':'#','&':'&','+':'+','=':'=','?':'?','[':'[',']':']','^':'^','`':'`','{':'{','|':'|','}':'}')";

Review comment:
       Can we make this more readable?

##########
File path: superset-frontend/src/views/CRUD/utils.tsx
##########
@@ -33,6 +33,35 @@ import { FetchDataConfig } from 'src/components/ListView';
 import SupersetText from 'src/utils/textUtils';
 import { Dashboard, Filters } from './types';
 
+// Modifies the rison encoding slightly to match the backend's
+// rison encoding/decoding. Applies globally. Code pulled from rison.js

Review comment:
       Nit, can you add here that `rison.js` is licensed under the MIT license, so we know we can reuse it? We also need to have a link to the original project, since the MIT license requires crediting.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] hughhhh commented on a change in pull request #18056: fix(CRUD/listviews): Errors with rison and search strings using special characters

Posted by GitBox <gi...@apache.org>.
hughhhh commented on a change in pull request #18056:
URL: https://github.com/apache/superset/pull/18056#discussion_r806337688



##########
File path: superset-frontend/src/views/CRUD/utils.tsx
##########
@@ -33,6 +33,35 @@ import { FetchDataConfig } from 'src/components/ListView';
 import SupersetText from 'src/utils/textUtils';
 import { Dashboard, Filters } from './types';
 
+// Modifies the rison encoding slightly to match the backend's
+// rison encoding/decoding. Applies globally. Code pulled from rison.js
+(() => {

Review comment:
       is this called everytime `rison.encode_uri` is called?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] corbinrobb commented on a change in pull request #18056: fix(CRUD/listviews): Errors with rison and search strings using special characters

Posted by GitBox <gi...@apache.org>.
corbinrobb commented on a change in pull request #18056:
URL: https://github.com/apache/superset/pull/18056#discussion_r806972575



##########
File path: superset-frontend/src/views/CRUD/utils.test.tsx
##########
@@ -171,3 +172,21 @@ test('does not ask for password when the import type is wrong', () => {
   };
   expect(hasTerminalValidation(error.errors)).toBe(true);
 });
+
+test('successfully modified rison to encode correctly', () => {
+  const problemCharacters = '& # ? ^ { } [ ] | " = + `';
+
+  const testObject = problemCharacters.split(' ').reduce((a, c) => {
+    // eslint-disable-next-line no-param-reassign
+    a[c] = c;
+    return a;
+  }, {});
+
+  const actualEncoding = rison.encode(testObject);
+
+  const expectedEncoding =
+    "('\"':'\"','#':'#','&':'&','+':'+','=':'=','?':'?','[':'[',']':']','^':'^','`':'`','{':'{','|':'|','}':'}')";

Review comment:
       I can have it make and encode an object for each one of the characters being tested instead of having it all in one. It will get rid of this big string and should be easier to read. I am pushing it up now so let me know if it needs more work and I can give it another go




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] codecov[bot] edited a comment on pull request #18056: fix(CRUD/listviews): Errors with rison and search strings using special characters

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #18056:
URL: https://github.com/apache/superset/pull/18056#issuecomment-1029314276






-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] github-actions[bot] commented on pull request #18056: fix(CRUD/listviews): Errors with rison and search strings using special characters

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #18056:
URL: https://github.com/apache/superset/pull/18056#issuecomment-1030240151


   @pkdotson Ephemeral environment spinning up at http://34.212.43.212:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] codecov[bot] edited a comment on pull request #18056: fix(CRUD/listviews): Errors with rison and search strings using special characters

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #18056:
URL: https://github.com/apache/superset/pull/18056#issuecomment-1029314276


   # [Codecov](https://codecov.io/gh/apache/superset/pull/18056?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#18056](https://codecov.io/gh/apache/superset/pull/18056?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5b0cd05) into [master](https://codecov.io/gh/apache/superset/commit/14b9298ef72e73372c2d3f3b1f9f5a1cfb064e1d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (14b9298) will **decrease** coverage by `0.05%`.
   > The diff coverage is `n/a`.
   
   > :exclamation: Current head 5b0cd05 differs from pull request most recent head eee8975. Consider uploading reports for the commit eee8975 to get more accurate results
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/18056/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/superset/pull/18056?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #18056      +/-   ##
   ==========================================
   - Coverage   66.34%   66.28%   -0.06%     
   ==========================================
     Files        1569     1594      +25     
     Lines       61685    62637     +952     
     Branches     6240     6314      +74     
   ==========================================
   + Hits        40927    41522     +595     
   - Misses      19161    19467     +306     
   - Partials     1597     1648      +51     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | javascript | `51.33% <ø> (+0.41%)` | :arrow_up: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/18056?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [superset/examples/multi\_line.py](https://codecov.io/gh/apache/superset/pull/18056/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZXhhbXBsZXMvbXVsdGlfbGluZS5weQ==) | `0.00% <0.00%> (-53.85%)` | :arrow_down: |
   | [superset/commands/importers/v1/examples.py](https://codecov.io/gh/apache/superset/pull/18056/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29tbWFuZHMvaW1wb3J0ZXJzL3YxL2V4YW1wbGVzLnB5) | `0.00% <0.00%> (-38.64%)` | :arrow_down: |
   | [superset/examples/big\_data.py](https://codecov.io/gh/apache/superset/pull/18056/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZXhhbXBsZXMvYmlnX2RhdGEucHk=) | `0.00% <0.00%> (-35.00%)` | :arrow_down: |
   | [superset/examples/misc\_dashboard.py](https://codecov.io/gh/apache/superset/pull/18056/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZXhhbXBsZXMvbWlzY19kYXNoYm9hcmQucHk=) | `0.00% <0.00%> (-33.34%)` | :arrow_down: |
   | [superset/examples/utils.py](https://codecov.io/gh/apache/superset/pull/18056/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZXhhbXBsZXMvdXRpbHMucHk=) | `0.00% <0.00%> (-28.58%)` | :arrow_down: |
   | [superset/examples/tabbed\_dashboard.py](https://codecov.io/gh/apache/superset/pull/18056/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZXhhbXBsZXMvdGFiYmVkX2Rhc2hib2FyZC5weQ==) | `0.00% <0.00%> (-27.59%)` | :arrow_down: |
   | [superset/db\_engine\_specs/teradata.py](https://codecov.io/gh/apache/superset/pull/18056/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3RlcmFkYXRhLnB5) | `62.75% <0.00%> (-27.25%)` | :arrow_down: |
   | [superset/examples/bart\_lines.py](https://codecov.io/gh/apache/superset/pull/18056/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZXhhbXBsZXMvYmFydF9saW5lcy5weQ==) | `0.00% <0.00%> (-25.81%)` | :arrow_down: |
   | [superset/utils/mock\_data.py](https://codecov.io/gh/apache/superset/pull/18056/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvbW9ja19kYXRhLnB5) | `0.00% <0.00%> (-25.19%)` | :arrow_down: |
   | [superset/examples/paris.py](https://codecov.io/gh/apache/superset/pull/18056/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZXhhbXBsZXMvcGFyaXMucHk=) | `0.00% <0.00%> (-25.00%)` | :arrow_down: |
   | ... and [202 more](https://codecov.io/gh/apache/superset/pull/18056/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/18056?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/18056?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [14b9298...eee8975](https://codecov.io/gh/apache/superset/pull/18056?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] betodealmeida commented on a change in pull request #18056: fix(CRUD/listviews): Errors with rison and search strings using special characters

Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #18056:
URL: https://github.com/apache/superset/pull/18056#discussion_r806390564



##########
File path: superset-frontend/src/views/CRUD/utils.test.tsx
##########
@@ -171,3 +172,21 @@ test('does not ask for password when the import type is wrong', () => {
   };
   expect(hasTerminalValidation(error.errors)).toBe(true);
 });
+
+test('successfully modified rison to encode correctly', () => {
+  const problemCharacters = '& # ? ^ { } [ ] | " = + `';
+
+  const testObject = problemCharacters.split(' ').reduce((a, c) => {
+    // eslint-disable-next-line no-param-reassign
+    a[c] = c;
+    return a;
+  }, {});
+
+  const actualEncoding = rison.encode(testObject);
+
+  const expectedEncoding =
+    "('\"':'\"','#':'#','&':'&','+':'+','=':'=','?':'?','[':'[',']':']','^':'^','`':'`','{':'{','|':'|','}':'}')";

Review comment:
       Can we make this more readable?

##########
File path: superset-frontend/src/views/CRUD/utils.tsx
##########
@@ -33,6 +33,35 @@ import { FetchDataConfig } from 'src/components/ListView';
 import SupersetText from 'src/utils/textUtils';
 import { Dashboard, Filters } from './types';
 
+// Modifies the rison encoding slightly to match the backend's
+// rison encoding/decoding. Applies globally. Code pulled from rison.js

Review comment:
       Nit, can you add here that `rison.js` is licensed under the MIT license, so we know we can reuse it? We also need to have a link to the original project, since the MIT license requires crediting.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] lyndsiWilliams merged pull request #18056: fix(CRUD/listviews): Errors with rison and search strings using special characters

Posted by GitBox <gi...@apache.org>.
lyndsiWilliams merged pull request #18056:
URL: https://github.com/apache/superset/pull/18056


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] cancan101 commented on pull request #18056: fix(CRUD/listviews): Errors with rison and search strings using special characters

Posted by GitBox <gi...@apache.org>.
cancan101 commented on pull request #18056:
URL: https://github.com/apache/superset/pull/18056#issuecomment-1055926969


   Should this PR also have updated this usage too:? https://github.com/apache/superset/blob/2491b89f2911482d9951064e541d616e396f75eb/superset-frontend/src/components/Datasource/DatasourceEditor.jsx#L642-L644


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] corbinrobb commented on a change in pull request #18056: fix(CRUD/listviews): Errors with rison and search strings using special characters

Posted by GitBox <gi...@apache.org>.
corbinrobb commented on a change in pull request #18056:
URL: https://github.com/apache/superset/pull/18056#discussion_r806377333



##########
File path: superset-frontend/src/views/CRUD/utils.tsx
##########
@@ -33,6 +33,35 @@ import { FetchDataConfig } from 'src/components/ListView';
 import SupersetText from 'src/utils/textUtils';
 import { Dashboard, Filters } from './types';
 
+// Modifies the rison encoding slightly to match the backend's
+// rison encoding/decoding. Applies globally. Code pulled from rison.js
+(() => {

Review comment:
       This function should only run once and since it doesn't get named, exported, or called anywhere else, it shouldn't run again. This just takes advantage of the rison object being global and mutable, and changes the regex variable on it that decides what to do with certain characters when it encodes/decodes. That regex variable gets used every time encode_uri, encode or decode is called but this function itself doesn't




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] codecov[bot] edited a comment on pull request #18056: fix(CRUD/listviews): Errors with rison and search strings using special characters

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #18056:
URL: https://github.com/apache/superset/pull/18056#issuecomment-1029314276


   # [Codecov](https://codecov.io/gh/apache/superset/pull/18056?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#18056](https://codecov.io/gh/apache/superset/pull/18056?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a91bba7) into [master](https://codecov.io/gh/apache/superset/commit/14b9298ef72e73372c2d3f3b1f9f5a1cfb064e1d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (14b9298) will **decrease** coverage by `0.03%`.
   > The diff coverage is `n/a`.
   
   > :exclamation: Current head a91bba7 differs from pull request most recent head eee8975. Consider uploading reports for the commit eee8975 to get more accurate results
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/18056/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/superset/pull/18056?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #18056      +/-   ##
   ==========================================
   - Coverage   66.34%   66.31%   -0.04%     
   ==========================================
     Files        1569     1620      +51     
     Lines       61685    63075    +1390     
     Branches     6240     6370     +130     
   ==========================================
   + Hits        40927    41827     +900     
   - Misses      19161    19591     +430     
   - Partials     1597     1657      +60     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | javascript | `51.24% <ø> (+0.32%)` | :arrow_up: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/18056?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [superset/examples/multi\_line.py](https://codecov.io/gh/apache/superset/pull/18056/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZXhhbXBsZXMvbXVsdGlfbGluZS5weQ==) | `0.00% <0.00%> (-53.85%)` | :arrow_down: |
   | [superset/commands/importers/v1/examples.py](https://codecov.io/gh/apache/superset/pull/18056/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29tbWFuZHMvaW1wb3J0ZXJzL3YxL2V4YW1wbGVzLnB5) | `0.00% <0.00%> (-38.64%)` | :arrow_down: |
   | [superset/examples/big\_data.py](https://codecov.io/gh/apache/superset/pull/18056/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZXhhbXBsZXMvYmlnX2RhdGEucHk=) | `0.00% <0.00%> (-35.00%)` | :arrow_down: |
   | [superset/examples/misc\_dashboard.py](https://codecov.io/gh/apache/superset/pull/18056/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZXhhbXBsZXMvbWlzY19kYXNoYm9hcmQucHk=) | `0.00% <0.00%> (-33.34%)` | :arrow_down: |
   | [superset/examples/utils.py](https://codecov.io/gh/apache/superset/pull/18056/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZXhhbXBsZXMvdXRpbHMucHk=) | `0.00% <0.00%> (-28.58%)` | :arrow_down: |
   | [superset/examples/tabbed\_dashboard.py](https://codecov.io/gh/apache/superset/pull/18056/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZXhhbXBsZXMvdGFiYmVkX2Rhc2hib2FyZC5weQ==) | `0.00% <0.00%> (-27.59%)` | :arrow_down: |
   | [superset/db\_engine\_specs/teradata.py](https://codecov.io/gh/apache/superset/pull/18056/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3RlcmFkYXRhLnB5) | `62.75% <0.00%> (-27.25%)` | :arrow_down: |
   | [superset/examples/bart\_lines.py](https://codecov.io/gh/apache/superset/pull/18056/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZXhhbXBsZXMvYmFydF9saW5lcy5weQ==) | `0.00% <0.00%> (-25.81%)` | :arrow_down: |
   | [superset/utils/mock\_data.py](https://codecov.io/gh/apache/superset/pull/18056/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvbW9ja19kYXRhLnB5) | `0.00% <0.00%> (-25.19%)` | :arrow_down: |
   | [superset/examples/paris.py](https://codecov.io/gh/apache/superset/pull/18056/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZXhhbXBsZXMvcGFyaXMucHk=) | `0.00% <0.00%> (-25.00%)` | :arrow_down: |
   | ... and [322 more](https://codecov.io/gh/apache/superset/pull/18056/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/18056?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/18056?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [14b9298...eee8975](https://codecov.io/gh/apache/superset/pull/18056?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] codecov[bot] commented on pull request #18056: fix(CRUD/listviews): Errors with rison and search strings using special characters

Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #18056:
URL: https://github.com/apache/superset/pull/18056#issuecomment-1029314276


   # [Codecov](https://codecov.io/gh/apache/superset/pull/18056?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#18056](https://codecov.io/gh/apache/superset/pull/18056?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5b0cd05) into [master](https://codecov.io/gh/apache/superset/commit/14b9298ef72e73372c2d3f3b1f9f5a1cfb064e1d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (14b9298) will **decrease** coverage by `0.05%`.
   > The diff coverage is `n/a`.
   
   > :exclamation: Current head 5b0cd05 differs from pull request most recent head d88c4fe. Consider uploading reports for the commit d88c4fe to get more accurate results
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/18056/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/superset/pull/18056?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #18056      +/-   ##
   ==========================================
   - Coverage   66.34%   66.28%   -0.06%     
   ==========================================
     Files        1569     1594      +25     
     Lines       61685    62637     +952     
     Branches     6240     6314      +74     
   ==========================================
   + Hits        40927    41522     +595     
   - Misses      19161    19467     +306     
   - Partials     1597     1648      +51     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | javascript | `51.33% <ø> (+0.41%)` | :arrow_up: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/18056?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [superset/examples/multi\_line.py](https://codecov.io/gh/apache/superset/pull/18056/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZXhhbXBsZXMvbXVsdGlfbGluZS5weQ==) | `0.00% <0.00%> (-53.85%)` | :arrow_down: |
   | [superset/commands/importers/v1/examples.py](https://codecov.io/gh/apache/superset/pull/18056/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29tbWFuZHMvaW1wb3J0ZXJzL3YxL2V4YW1wbGVzLnB5) | `0.00% <0.00%> (-38.64%)` | :arrow_down: |
   | [superset/examples/big\_data.py](https://codecov.io/gh/apache/superset/pull/18056/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZXhhbXBsZXMvYmlnX2RhdGEucHk=) | `0.00% <0.00%> (-35.00%)` | :arrow_down: |
   | [superset/examples/misc\_dashboard.py](https://codecov.io/gh/apache/superset/pull/18056/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZXhhbXBsZXMvbWlzY19kYXNoYm9hcmQucHk=) | `0.00% <0.00%> (-33.34%)` | :arrow_down: |
   | [superset/examples/utils.py](https://codecov.io/gh/apache/superset/pull/18056/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZXhhbXBsZXMvdXRpbHMucHk=) | `0.00% <0.00%> (-28.58%)` | :arrow_down: |
   | [superset/examples/tabbed\_dashboard.py](https://codecov.io/gh/apache/superset/pull/18056/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZXhhbXBsZXMvdGFiYmVkX2Rhc2hib2FyZC5weQ==) | `0.00% <0.00%> (-27.59%)` | :arrow_down: |
   | [superset/db\_engine\_specs/teradata.py](https://codecov.io/gh/apache/superset/pull/18056/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3RlcmFkYXRhLnB5) | `62.75% <0.00%> (-27.25%)` | :arrow_down: |
   | [superset/examples/bart\_lines.py](https://codecov.io/gh/apache/superset/pull/18056/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZXhhbXBsZXMvYmFydF9saW5lcy5weQ==) | `0.00% <0.00%> (-25.81%)` | :arrow_down: |
   | [superset/utils/mock\_data.py](https://codecov.io/gh/apache/superset/pull/18056/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvbW9ja19kYXRhLnB5) | `0.00% <0.00%> (-25.19%)` | :arrow_down: |
   | [superset/examples/paris.py](https://codecov.io/gh/apache/superset/pull/18056/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZXhhbXBsZXMvcGFyaXMucHk=) | `0.00% <0.00%> (-25.00%)` | :arrow_down: |
   | ... and [202 more](https://codecov.io/gh/apache/superset/pull/18056/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/18056?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/18056?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [14b9298...d88c4fe](https://codecov.io/gh/apache/superset/pull/18056?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] pkdotson commented on pull request #18056: fix(CRUD/listviews): Errors with rison and search strings using special characters

Posted by GitBox <gi...@apache.org>.
pkdotson commented on pull request #18056:
URL: https://github.com/apache/superset/pull/18056#issuecomment-1030238943


   /testenv up


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] github-actions[bot] commented on pull request #18056: fix(CRUD/listviews): Errors with rison and search strings using special characters

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #18056:
URL: https://github.com/apache/superset/pull/18056#issuecomment-1040845881


   Ephemeral environment shutdown and build artifacts deleted.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] rosemarie-chiu commented on pull request #18056: fix(CRUD/listviews): Errors with rison and search strings using special characters

Posted by GitBox <gi...@apache.org>.
rosemarie-chiu commented on pull request #18056:
URL: https://github.com/apache/superset/pull/18056#issuecomment-1041924295


   🏷 preset:2022.7


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] pkdotson commented on pull request #18056: fix(CRUD/listviews): [WIP] errors with rison and search strings using special characters

Posted by GitBox <gi...@apache.org>.
pkdotson commented on pull request #18056:
URL: https://github.com/apache/superset/pull/18056#issuecomment-1027314150


   Hi @corbinrobb thanks for the initial fix. Is it possible to add some unit tests for this?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] corbinrobb commented on a change in pull request #18056: fix(CRUD/listviews): Errors with rison and search strings using special characters

Posted by GitBox <gi...@apache.org>.
corbinrobb commented on a change in pull request #18056:
URL: https://github.com/apache/superset/pull/18056#discussion_r806940057



##########
File path: superset-frontend/src/views/CRUD/utils.tsx
##########
@@ -33,6 +33,35 @@ import { FetchDataConfig } from 'src/components/ListView';
 import SupersetText from 'src/utils/textUtils';
 import { Dashboard, Filters } from './types';
 
+// Modifies the rison encoding slightly to match the backend's
+// rison encoding/decoding. Applies globally. Code pulled from rison.js

Review comment:
       Sure thing! I will have that pushed up soon




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] corbinrobb commented on a change in pull request #18056: fix(CRUD/listviews): Errors with rison and search strings using special characters

Posted by GitBox <gi...@apache.org>.
corbinrobb commented on a change in pull request #18056:
URL: https://github.com/apache/superset/pull/18056#discussion_r806377333



##########
File path: superset-frontend/src/views/CRUD/utils.tsx
##########
@@ -33,6 +33,35 @@ import { FetchDataConfig } from 'src/components/ListView';
 import SupersetText from 'src/utils/textUtils';
 import { Dashboard, Filters } from './types';
 
+// Modifies the rison encoding slightly to match the backend's
+// rison encoding/decoding. Applies globally. Code pulled from rison.js
+(() => {

Review comment:
       This function should only run once and since it doesn't get named, exported, or called anywhere else, it shouldn't run again. This just takes advantage of the rison object being global and mutable, and changes the regex variable on it that decides what to do with certain characters when it encodes/decodes. That regex variable gets used every time encode_uri, encode or decode is called but this function itself doesn't

##########
File path: superset-frontend/src/views/CRUD/utils.tsx
##########
@@ -33,6 +33,35 @@ import { FetchDataConfig } from 'src/components/ListView';
 import SupersetText from 'src/utils/textUtils';
 import { Dashboard, Filters } from './types';
 
+// Modifies the rison encoding slightly to match the backend's
+// rison encoding/decoding. Applies globally. Code pulled from rison.js

Review comment:
       Sure thing! I will have that pushed up soon

##########
File path: superset-frontend/src/views/CRUD/utils.test.tsx
##########
@@ -171,3 +172,21 @@ test('does not ask for password when the import type is wrong', () => {
   };
   expect(hasTerminalValidation(error.errors)).toBe(true);
 });
+
+test('successfully modified rison to encode correctly', () => {
+  const problemCharacters = '& # ? ^ { } [ ] | " = + `';
+
+  const testObject = problemCharacters.split(' ').reduce((a, c) => {
+    // eslint-disable-next-line no-param-reassign
+    a[c] = c;
+    return a;
+  }, {});
+
+  const actualEncoding = rison.encode(testObject);
+
+  const expectedEncoding =
+    "('\"':'\"','#':'#','&':'&','+':'+','=':'=','?':'?','[':'[',']':']','^':'^','`':'`','{':'{','|':'|','}':'}')";

Review comment:
       I can have it make and encode an object for each one of the characters being tested instead of having it all in one. It will get rid of this big string and should be easier to read. I am pushing it up now so let me know if it needs more work and I can give it another go




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] hughhhh commented on a change in pull request #18056: fix(CRUD/listviews): Errors with rison and search strings using special characters

Posted by GitBox <gi...@apache.org>.
hughhhh commented on a change in pull request #18056:
URL: https://github.com/apache/superset/pull/18056#discussion_r806337688



##########
File path: superset-frontend/src/views/CRUD/utils.tsx
##########
@@ -33,6 +33,35 @@ import { FetchDataConfig } from 'src/components/ListView';
 import SupersetText from 'src/utils/textUtils';
 import { Dashboard, Filters } from './types';
 
+// Modifies the rison encoding slightly to match the backend's
+// rison encoding/decoding. Applies globally. Code pulled from rison.js
+(() => {

Review comment:
       is this called everytime `rison.encode_uri` is called?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org