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

[selenium manager]: fix edge artifact deserialisation #14859

Merged
merged 5 commits into from
Dec 11, 2024

Conversation

Delta456
Copy link
Contributor

@Delta456 Delta456 commented Dec 5, 2024

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Fixes #14851

Motivation and Context

The latest API of Microsoft Edge product updates now has renamed some fields which causes a lot of tests and selenium manager to fail.

API: https://edgeupdates.microsoft.com/api/products/

Previous:

       "Artifacts": [
          {
            "ArtifactName": "msi",
            "Location": "https://msedge.sf.dl.delivery.mp.microsoft.com/filestreamingservice/files/91731797-32c8-494b-ba04-1083d8ebaf73/MicrosoftEdgeEnterpriseX64.msi",
            "Hash": "15F1ECC09251FF7D516B5B1BD4B48E07434A0546C6FB8822BEED1026DB5D4975",
            "HashAlgorithm": "SHA256",
            "SizeInBytes": 182636544
          }

Now:

       "Artifacts": [
          {
            "artifactName": "msi",
            "location": "https://msedge.sf.dl.delivery.mp.microsoft.com/filestreamingservice/files/91731797-32c8-494b-ba04-1083d8ebaf73/MicrosoftEdgeEnterpriseX64.msi",
            "hash": "15F1ECC09251FF7D516B5B1BD4B48E07434A0546C6FB8822BEED1026DB5D4975",
            "hashAlgorithm": "SHA256",
            "sizeInBytes": 182636544
          }

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Bug fix


Description

  • Fixed deserialization issues in the Selenium Manager for Microsoft Edge by updating field names in the Artifact struct to match the latest API response from Microsoft.
  • This change addresses the JSON parsing error due to renamed fields in the Edge product updates API.

Changes walkthrough 📝

Relevant files
Bug fix
edge.rs
Update field names in `Artifact` struct for Edge API compatibility

rust/src/edge.rs

  • Updated field names in the Artifact struct to match the new API
    response.
  • Changed ArtifactName to artifactName.
  • Changed Location to location.
  • Changed Hash to hash.
  • Changed HashAlgorithm to hashAlgorithm.
  • Changed SizeInBytes to sizeInBytes.
  • +5/-5     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link
    Contributor

    qodo-merge-pro bot commented Dec 5, 2024

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    14851 - Fully compliant

    Compliant requirements:

    • Updates field names in Artifact struct to match new API response format
    • Fixes JSON parsing error by using correct field names
    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    API Compatibility
    Verify that all possible Edge API response fields are properly renamed and that no fields are missing

    Copy link
    Contributor

    qodo-merge-pro bot commented Dec 5, 2024

    PR Code Suggestions ✨

    No code suggestions found for the PR.

    @harsha509 harsha509 requested a review from bonigarcia December 5, 2024 18:38
    @Fman77
    Copy link

    Fman77 commented Dec 5, 2024

    What if they switch again to uppercase? Shouldn't we make this case-insensitive?

    Is there any way we can make the fix work for previous versions of selenium? Could we contact the MSEdge team and tell them they break Selenium, so that they can un-break it, until we have a newer version that handles case-insensitive attribute names?

    @VietND96
    Copy link
    Member

    VietND96 commented Dec 6, 2024

    Can we make this work for the key name with case-insensitive?

    @Delta456
    Copy link
    Contributor Author

    Delta456 commented Dec 6, 2024

    I am not sure if it's a good idea. Plus I don't believe Edge API will break now.

    @VietND96
    Copy link
    Member

    VietND96 commented Dec 6, 2024

    Since via response, all of the keys are not synced in the same standard. Assume one day they are going to change their mind to update the camel case for all other parent keys, we can not guard that.

    @Delta456
    Copy link
    Contributor Author

    Delta456 commented Dec 6, 2024

    Do we know all possible options from which they can change into? If yes then maybe we can work upon that.

    @bonigarcia
    Copy link
    Member

    I agree with @Fman77 and @VietND96. I believe the best solution for this issue will be to make this parsing case-insensitive (i.e., accept both ArtifactName and artifactName, etc.).

    @Delta456: Can you please look at the Rust code to make this case insensitive?

    Also, I reported this problem to the EdgeWebDriver issue tracker:

    MicrosoftEdge/EdgeWebDriver#178

    However, the MS Edge WebDriver team is not very responsive to this tracker. It is not the first time I have reported something there (see here), and I didn't have any response, so probably this time will be the same.

    Copy link
    Member

    @bonigarcia bonigarcia left a comment

    Choose a reason for hiding this comment

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

    Can we make this case-insentive?

    @Delta456
    Copy link
    Contributor Author

    Delta456 commented Dec 6, 2024

    I do not know the best way to make this case insensitive. What preferred way would you suggest?

    I also believe the other Struct fields can be renamed with time so we have to make that for all.

    @Delta456
    Copy link
    Contributor Author

    Delta456 commented Dec 8, 2024

    accept both ArtifactName and artifactName

    If we are only going with ArtifactName and artifactName then we can simply use #[serde(alias = "...")]. The following would be the change

    #[derive(Serialize, Deserialize, Debug)]
    pub struct Artifact {
        #[serde(rename = "ArtifactName", alias = "artifactName")]
        pub artifact_name: String,
        #[serde(rename = "Location", alias = "location")]
        pub location: String,
        #[serde(rename = "Hash", alias = "hash")]
        pub hash: String,
        #[serde(rename = "HashAlgorithm", alias = "hashAlgorithm")]
        pub hash_algorithm: String,
        #[serde(rename = "SizeInBytes", alias = "sizeInBytes")]
        pub size_in_bytes: u32,
    }

    @bonigarcia what do you think?

    @bonigarcia
    Copy link
    Member

    I've analyzed the required code for this. Indeed, doing this parsing case-insensitive is not straightforward with Rust's serde. In theory, it should be possible to use a custom deserializer and deserialize_with. Also, additional existing crates like serde_aux do a similar job. But the solution of using an alias is simple and covers the two (in theory) cases we need to face, i.e., ArtifactName and artifactName (and the other fields). So, I believe it is a good compromise for solving this issue. So please, @Delta456, include those changes in the PR, and I will merge them. Thanks a lot!

    @Delta456
    Copy link
    Contributor Author

    @bonigarcia should I also add an alias for Release and EdgeProduct structs as it can be changed someday soon.

    @bonigarcia
    Copy link
    Member

    I hope that change will not happen in the future. At least, the Microsoft team is now aware that that kind of change can break the clients of their API. But, yes, just in case, we can include a camel-case alias for all the members in those structs - thanks.

    @Delta456
    Copy link
    Contributor Author

    Sure, I will do that in a while!

    @bonigarcia bonigarcia merged commit 622cb43 into SeleniumHQ:trunk Dec 11, 2024
    17 of 18 checks passed
    @bonigarcia
    Copy link
    Member

    Thanks, @Delta456!

    @Delta456 Delta456 deleted the selenium_manager branch December 12, 2024 03:36
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    [🐛 Bug]: Selenium Manager unable to lookup Edge - Error parsing JSON
    5 participants