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

🐛 Source Facebook Marketing: fix unpack ValueError in remove_params_from_url #5758

Conversation

vitaliizazmic
Copy link
Contributor

What

According to comment in issue #5190 jobs are failing in streams.remove_params_from_url:

 ERROR () LineGobbler(voidCall):85 -     record["thumbnail_url"] = remove_params_from_url(thumbnail_url, ["_nc_hash", "d"])
2021-08-03 20:02:54 ERROR () LineGobbler(voidCall):85 -   File "/airbyte/integration_code/source_facebook_marketing/streams.py", line 53, in remove_params_from_url
2021-08-03 20:02:54 ERROR () LineGobbler(voidCall):85 -     key, value = q.split("=")
2021-08-03 20:02:54 ERROR () LineGobbler(voidCall):85 - ValueError: not enough values to unpack (expected 2, got 1)

remove_params_from_url method is used while syncing ad creatives streams. Its records contains thumbnail url, which have query parameters. Some URLs has random values, these values doesn't affect validity of URLs, but breaks SAT, therefore parameters with this values are removed. Error occurred when parameters didn't have values. Additional checking was added.

@github-actions github-actions bot added the area/connectors Connector related issues label Aug 31, 2021
@vitaliizazmic vitaliizazmic self-assigned this Aug 31, 2021
@vitaliizazmic
Copy link
Contributor Author

vitaliizazmic commented Aug 31, 2021

/test connector=source-facebook-marketing

🕑 source-facebook-marketing https://github.com/airbytehq/airbyte/actions/runs/1186442299
❌ source-facebook-marketing https://github.com/airbytehq/airbyte/actions/runs/1186442299

@jrhizor jrhizor temporarily deployed to more-secrets August 31, 2021 14:07 Inactive
Copy link
Contributor

@gaart gaart left a comment

Choose a reason for hiding this comment

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

Probably should use more generic solution

if len(q.split("=")) == 2:
key, value = q.split("=")
if key not in params:
res_query.append(f"{key}={value}")
Copy link
Contributor

Choose a reason for hiding this comment

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

from urllib.parse import urlparse, parse_qs, urlunparse, urlencode

def remove_params_from_url(url, params):
    """
    >>> remove_params_from_url('https://foo.bar/?x=1&y=&z=3', params=['x'])
    'https://foo.bar/?y=&z=3'
    >>> remove_params_from_url('https://foo.bar/?x=1&y=&z=3', params=['y'])
    'https://foo.bar/?x=1&z=3'
    >>> remove_params_from_url('https://foo.bar/?x=1&y=&z=3', params=['x', 'z'])
    'https://foo.bar/?y='
    >>> remove_params_from_url('https://foo.bar/?x=1&y=&z=3&x=111', params=['z'])
    'https://foo.bar/?x=1&x=111&y='
    """
    parsed = urlparse(url)
    query = parse_qs(parsed.query, keep_blank_values=True)
    filtered = dict((k, v) for k, v in query.items() if k not in params)
    return urlunparse([
        parsed.scheme,
        parsed.netloc,
        parsed.path,
        parsed.params,
        urlencode(filtered, doseq=True),
        parsed.fragment
    ])

@CLAassistant
Copy link

CLAassistant commented Sep 27, 2021

CLA assistant check
All committers have signed the CLA.

@vitaliizazmic
Copy link
Contributor Author

It was fixed in PR #5958

@swyxio swyxio deleted the vitalii/5190_facebook_marketing_remove_params_from_url_value_error branch October 12, 2022 18:19
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.

None yet

5 participants