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

Operator's extra link button is disabled #43252

Open
1 of 2 tasks
mfatemipour opened this issue Oct 22, 2024 · 9 comments
Open
1 of 2 tasks

Operator's extra link button is disabled #43252

mfatemipour opened this issue Oct 22, 2024 · 9 comments
Labels
affected_version:2.10 Issues Reported for 2.10 area:core area:UI Related to UI/UX. For Frontend Developers. good first issue kind:bug This is a clearly a bug

Comments

@mfatemipour
Copy link
Contributor

mfatemipour commented Oct 22, 2024

Apache Airflow version

2.10.2

What happened?

When in get_link method I provide None, the button is enabled and when I provide link (internal link) it is disabled

What you think should happen instead?

In the following code, if url is empty or None, the result of isSanitised is True !
also if the url is internal then isSanitised is False (so internal url is not supported)

As a workaround for now I am providing a random non-rul string instead of None, and I replaced my internal url with absolute one (but I missed rendering my page in same window now)

https://github.com/apache/airflow/blob/2.10.2/airflow/www/static/js/dag/details/taskInstance/ExtraLinks.tsx

const isExternal = (url: string | null) =>
    url && /^(?:[a-z]+:)?\/\//.test(url);

  const isSanitised = (url: string | null) => {
    if (!url) {
      return true;
    }
    const urlRegex = /^(https?:)/i;
    return urlRegex.test(url);
  };

  return (
    <Box my={3}>
      <Text as="strong">Extra Links</Text>
      <Flex flexWrap="wrap" mt={3}>
        {links.map(({ name, url }) => (
          <Button
            key={name}
            as={Link}
            colorScheme="blue"
            href={url}
            isDisabled={!isSanitised(url)}
            target={isExternal(url) ? "_blank" : undefined}
            mr={2}
          >
            {name}
          </Button>
        ))}
      </Flex>
      <Divider my={2} />
    </Box>
  );
};

How to reproduce

  • Create a simple plugin with an OperatorLink
  • Return None in get_link() method of your OperatorLink
  • You can see the button in Details page of corresponding task is enalbed.

Operating System

Debian GNU/Linux 12 (bookworm)

Versions of Apache Airflow Providers

non related

Deployment

Docker-Compose

Deployment details

We are deploying on docker swarm

Anything else?

It works in 2.8.1
And It seems that internal url issue is fixed in main branch but None handling is not fixed.

Are you willing to submit PR?

  • Yes I am willing to submit a PR!

Code of Conduct

@mfatemipour mfatemipour added area:core kind:bug This is a clearly a bug needs-triage label for new issues that we didn't triage yet labels Oct 22, 2024
@dosubot dosubot bot added the area:UI Related to UI/UX. For Frontend Developers. label Oct 22, 2024
@potiuk potiuk added good first issue and removed needs-triage label for new issues that we didn't triage yet labels Oct 24, 2024
@enisnazif
Copy link
Contributor

I'm happy to investigate this - will take a look now

@enisnazif
Copy link
Contributor

enisnazif commented Nov 9, 2024

If I understand you correctly, seems like this can be solved with:

const isSanitised = (url: string | null) => {
    if (!url) {
      return false;
    }
    const urlRegex = /^(https?:)/i;
    return urlRegex.test(url);
  };

i.e, if the URL is '' or null, set isSanitised to false so that the button ends up having the isDisabled prop set to true

@enisnazif
Copy link
Contributor

candidate PR here: #43844

potiuk pushed a commit that referenced this issue Nov 9, 2024
* Disable button if link is null or empty

* Fix space

---------

Co-authored-by: Enis Nazif <[email protected]>
jscheffl pushed a commit to jscheffl/airflow that referenced this issue Nov 9, 2024
…che#43844)

* Disable button if link is null or empty

* Fix space

---------

Co-authored-by: Enis Nazif <[email protected]>
(cherry picked from commit de88182)
jscheffl added a commit that referenced this issue Nov 9, 2024
…43851)

* Disable button if link is null or empty

* Fix space

---------

Co-authored-by: Enis Nazif <[email protected]>
(cherry picked from commit de88182)

Co-authored-by: enisnazif <[email protected]>
@mfatemipour
Copy link
Contributor Author

mfatemipour commented Nov 10, 2024

isSanitised should return true for internal urls too (urls that doesn't start with http).

@eladkal eladkal added the affected_version:2.10 Issues Reported for 2.10 label Nov 10, 2024
@eladkal
Copy link
Contributor

eladkal commented Nov 10, 2024

Fixed in #43844

ellisms pushed a commit to ellisms/airflow that referenced this issue Nov 13, 2024
…che#43844)

* Disable button if link is null or empty

* Fix space

---------

Co-authored-by: Enis Nazif <[email protected]>
@mfatemipour
Copy link
Contributor Author

Hi, Your fix isn't complete, as I mentioned in my previous command isSanitised should returns true also for internal urls. but now it returns false

const urlRegex = /^(https?:)/i;
return urlRegex.test(url);

@potiuk
Copy link
Member

potiuk commented Nov 28, 2024

const urlRegex = /^(https?:)/i;

Can you propose a better fix ?

@mfatemipour
Copy link
Contributor Author

mfatemipour commented Nov 28, 2024

Yeah, first of all, following code to check that a url is external is buggy:

const isExternal = (url: string | null) =>
    url && /^(?:[a-z]+:)?\/\//.test(url);

This regex will accept a url starting with "//"

so first let's fix this (removing ?):

const isExternal = (url: string | null) =>
    url && /^(?:[a-z]+:)\/\//.test(url);

And for the isSantised method let's reuse this isExternal
to check that if url is external or internal (for internal check, starts with a single / character)

const isSanitised = (url: string | null) => {
    if (!url) {
      return true;
    }
    return isExternal(url) || /^\/[^\/]/.test(url);
  };

I cannot test it, I don't have a ready airflow dev environment on my work laptop, otherwise I would be appreciated to contribute.

@potiuk
Copy link
Member

potiuk commented Nov 28, 2024

I cannot test it, I don't have a ready airflow dev environment on my work laptop, otherwise I would be appreciated to contribute.

You can use codespaces - which is entirely from the browser, supported by Airflow and free up to 80 hours a month for everyone in GitHub. Super easy to use - you click a button and you have working development environment with airflow in a matter of minutes. Quick start here: https://github.com/apache/airflow/blob/main/contributing-docs/quick-start-ide/contributors_quick_start_codespaces.rst

If you can create and coment on issues in GitHub, you can also run this development environment -as it is purely in-browser and runs Airflow's CI image remotely on GitHub's workers.

utkarsharma2 pushed a commit that referenced this issue Dec 4, 2024
…43851)

* Disable button if link is null or empty

* Fix space

---------

Co-authored-by: Enis Nazif <[email protected]>
(cherry picked from commit de88182)

Co-authored-by: enisnazif <[email protected]>
utkarsharma2 pushed a commit that referenced this issue Dec 9, 2024
…43851)

* Disable button if link is null or empty

* Fix space

---------

Co-authored-by: Enis Nazif <[email protected]>
(cherry picked from commit de88182)

Co-authored-by: enisnazif <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affected_version:2.10 Issues Reported for 2.10 area:core area:UI Related to UI/UX. For Frontend Developers. good first issue kind:bug This is a clearly a bug
Projects
None yet
Development

No branches or pull requests

4 participants