villebro opened a new pull request, #24166:
URL: https://github.com/apache/superset/pull/24166

   ### SUMMARY
   A recent PR #23888  that migrated serialization of permalink entries from 
Pickle to JSON caused a regression when encoding permalinks containing URL 
params. More specifically, this happened due to the fact that we were encoding 
the value with a JSON codec, which doesn't distinguish between tuples and 
lists. As the `urlParams` property in the permalink state was of the format 
`Optional[List[Tuple[str, str]]]`, they became `Optional[List[List[str]]]` 
after deserialization, which `urllib.parse.urlencode` is unable to process.
   
   To get around this we introduce a new codec called 
`MarshmallowKeyValueCodec`. This extends the `JsonKeyValueCodec` but takes a 
Marshmallow Schema in the constructor, and uses that during encoding/decoding. 
This ensures that a the entry that has been serialized in JSON format can 
contain tuples after decoding.
    
   ### AFTER
   Now permalinks containing `urlParams` work as expected:
   <img width="1305" alt="image" 
src="https://github.com/apache/superset/assets/33317356/2d6d5fb0-e2b3-4c67-96b3-e0dfd345e149";>
   
   ### BEFORE
   Previously permalinks with URL params would throw a 500:
   ```json
   {
     "errors": [
       {
         "message": "not a valid non-string sequence or mapping object", 
         "error_type": "GENERIC_BACKEND_ERROR", 
         "level": "error",
         "extra": {
           "issue_codes": [
             {
               "code": 1011, 
               "message": "Issue 1011 - Superset encountered an unexpected 
error."
              }
           ]
         }
       }
     ]
   }
   ```
   
   ### TESTING INSTRUCTIONS
   1. Create a Dashboard or Explore permalink containing a URL param `abc=123`
   2. Navigate to the permalink and notice the error
   
   ### 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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to