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

[Projects] Fix BC for GET requests of non-normalized function names #3568

Merged
merged 7 commits into from
May 23, 2023

Conversation

yaelgen
Copy link
Collaborator

@yaelgen yaelgen commented May 16, 2023

follow up for: #3287
In this previous PR we added a normalization to a function name before saving it, and notify the user of the change.

We realized that in case our users ignored/missed the warning when setting the function, and then attempted to access (import or update) it later, they would be unable to do so using the old name, and would not know why their function was not found.

As a result, as long as we do not block requests with non-normalized names, we will normalize GET requests as well.
In this fix we handle this case by adding fall-back to get_function:
Normalize the requested name and attempt to retrieve it from the database. if no response is received, we will check to see if the original name contained underscores, if so, we will repeat the retrieval and return the result (if exists).

The backwards compatibility behavior here is:
In circumstances when the user has an old function with non-normalized name already saved in the database, he will be able to retrieve it.
If he makes an update request, we will create a new updated function, with a normalized name. Now if he attempts to retrieve it using the old name, we will first normalize the name in the request for him, and return the updated function.

Copy link
Contributor

@Tankilevitch Tankilevitch left a comment

Choose a reason for hiding this comment

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

Nice Job! very concise and straight to the point @yaelgen

mlrun/api/db/sqldb/db.py Outdated Show resolved Hide resolved
mlrun/projects/project.py Outdated Show resolved Hide resolved
tests/api/api/test_projects.py Outdated Show resolved Hide resolved
tests/projects/test_project.py Show resolved Hide resolved
tests/projects/test_project.py Show resolved Hide resolved
@yaelgen yaelgen marked this pull request as ready for review May 21, 2023 09:04
Copy link
Member

@liranbg liranbg left a comment

Choose a reason for hiding this comment

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

Well done! few things

  1. Add type hint / param explanation for public functions you encounter and change, will help future developers
  2. Public functions before private functions, try to push down the internal implementation of get function after all public class apis
  3. It is legitimate to log that you function was not found, trying to find function without normalizing its name

Copy link
Member

@liranbg liranbg left a comment

Choose a reason for hiding this comment

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

last one

mlrun/api/db/sqldb/db.py Outdated Show resolved Hide resolved
@yaelgen yaelgen requested a review from Tankilevitch May 23, 2023 12:35
Copy link
Contributor

@Tankilevitch Tankilevitch left a comment

Choose a reason for hiding this comment

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

LGTM

mlrun/projects/project.py Outdated Show resolved Hide resolved
Copy link
Contributor

@Tankilevitch Tankilevitch left a comment

Choose a reason for hiding this comment

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

LGTM

Co-authored-by: Tom Tankilevitch <59158507+Tankilevitch@users.noreply.github.com>
@liranbg liranbg merged commit 5796e80 into mlrun:development May 23, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants