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

feat(event_handler): mark API operation as deprecated for OpenAPI documentation #5732

Merged

Conversation

tcysin
Copy link
Contributor

@tcysin tcysin commented Dec 12, 2024

Issue number: #5674

Summary

Add support for marking API operations as deprecated for OpenAPI documentation.

Changes

  • New deprecated parameter with default value False for all named decorators (get, post, put, delete, patch, head) in ApiGatewayResolver, ApiGatewayRestResolver, BedrockAgentResolver and Router.
  • Workaround to support backward-compatible interface in include_router.
  • Use collections.defaultdict.

User experience

from aws_lambda_powertools.event_handler import APIGatewayRestResolver
from aws_lambda_powertools.event_handler.api_gateway import Router


app = APIGatewayRestResolver()

@app.get("/eggs")  # deprecated=False by default
def eggs():
    return {"message": "cheese"}

@app.get("/spam", deprecated=True)
def spam():
    return {"message": "mmm... tasty"}

# Also works with router
router = Router()

@router.get("/ham", deprecated=True)
def ham():
    return {"message": "ham!"}

app.include_router(router, prefix="/blah")

Checklist

If your change doesn't seem to apply, please leave them unchecked.

Is this a breaking change?

RFC issue number:

Checklist:

  • Migration process documented
  • Implement warnings (if it can live side by side)

Acknowledgment

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

Copy link

boring-cyborg bot commented Dec 12, 2024

Thanks a lot for your first contribution! Please check out our contributing guidelines and don't hesitate to ask whatever you need.
In the meantime, check out the #python channel on our Powertools for AWS Lambda Discord: Invite link

@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 12, 2024
@tcysin
Copy link
Contributor Author

tcysin commented Dec 12, 2024

TODO

Testing:

  • End-to-end for all HTTP methods?
  • Resolvers & Router?

Documentation:

Copy link

codecov bot commented Dec 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.17%. Comparing base (086b3b7) to head (9968d68).
Report is 4 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #5732   +/-   ##
========================================
  Coverage    96.17%   96.17%           
========================================
  Files          231      231           
  Lines        10920    10922    +2     
  Branches      2019     2019           
========================================
+ Hits         10502    10504    +2     
  Misses         329      329           
  Partials        89       89           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tcysin
Copy link
Contributor Author

tcysin commented Dec 18, 2024

I would also ask for some help with adding an entry into docs/core/event_handler/_openapi_customization_operations.md, something like this:

Field Name Type Description
deprecated bool A boolean value that determines whether or not this operation should be marked as deprecated in the OpenAPI schema.

I cannot get make lint-docs-fix to work 😞

@tcysin tcysin marked this pull request as ready for review December 18, 2024 00:41
@tcysin tcysin requested a review from a team as a code owner December 18, 2024 00:41
@leandrodamascena
Copy link
Contributor

Hello everyone! I'm starting the review!

Thanks for your patience @tcysin. I'll check all your comments and respond.

Copy link
Contributor

@leandrodamascena leandrodamascena left a comment

Choose a reason for hiding this comment

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

Hi @tcysin! Thanks a lot for working on this, we completely missed that field. Sorry for the delay in responding and reviewing, but you know, the year is almost over and people are on annual leave.

Please let me know if you need help to address the comments.

@leandrodamascena
Copy link
Contributor

I would also ask for some help with adding an entry into docs/core/event_handler/_openapi_customization_operations.md, something like this:

Field Name Type Description
deprecated bool A boolean value that determines whether or not this operation should be marked as deprecated in the OpenAPI schema.
I cannot get make lint-docs-fix to work 😞

Hey, what is going on here? Can you please paste the error?

@tcysin
Copy link
Contributor Author

tcysin commented Dec 19, 2024

Oh my bad, make lint-docs-fix works fine. I think it's an issue with my local development environment.

I run Linux Mint 21.3 Cinnamon as an operating system, and installed Docker using the Software Manager.

👉 To run Docker commands, I must use sudo - I do not manage Docker as non-root user.

For this reason I need to temporarily add sudo to Docker commands in Makefile.

If I run Docker commands without sudo, I get this:

$ docker run hello-world
docker: permission denied while trying to connect to the Docker daemon socket at unix:///var/run/docker.sock: Post "http://%2Fvar%2Frun%2Fdocker.sock/v1.24/containers/create": dial unix /var/run/docker.sock: connect: permission denied.
See 'docker run --help'.

When I try to add the documentation and commit, I get this:

$ git commit -m "Add parameter description to Markdown docs"
[WARNING] Unstaged files detected.
[INFO] Stashing unstaged files to /home/tcysin/.cache/pre-commit/patch1734636743-94026.
check for merge conflicts................................................Passed
trim trailing whitespace.................................................Passed
check toml...........................................(no files to check)Skipped
formatting::black....................................(no files to check)Skipped
linting-format::ruff.................................(no files to check)Skipped
markdownlint-docker......................................................Failed
- hook id: markdownlint-docker
- exit code: 126

docker: permission denied while trying to connect to the Docker daemon socket at unix:///var/run/docker.sock: Post "http://%2Fvar%2Frun%2Fdocker.sock/v1.24/containers/create": dial unix /var/run/docker.sock: connect: permission denied.
See 'docker run --help'.

linting::cloudformation..............................(no files to check)Skipped
Lint GitHub Actions workflow file Docker.............(no files to check)Skipped
Terraform fmt........................................(no files to check)Skipped
Detect hardcoded secrets.................................................Passed
[INFO] Restored changes from /home/tcysin/.cache/pre-commit/patch1734636743-94026.

Pre-commit fails when running markdownlint-docker hook. Note the error message:

docker: permission denied while trying to connect to the Docker daemon socket at unix:///var/run/docker.sock: Post "http://%2Fvar%2Frun%2Fdocker.sock/v1.24/containers/create": dial unix /var/run/docker.sock: connect: permission denied.
See 'docker run --help'.

Here's me running that same hook directly:

$ pre-commit run markdownlint-docker --verbose
[WARNING] Unstaged files detected.
[INFO] Stashing unstaged files to /home/tcysin/.cache/pre-commit/patch1734636928-95070.
markdownlint-docker......................................................Failed
- hook id: markdownlint-docker
- duration: 0.02s
- exit code: 126

docker: permission denied while trying to connect to the Docker daemon socket at unix:///var/run/docker.sock: Post "http://%2Fvar%2Frun%2Fdocker.sock/v1.24/containers/create": dial unix /var/run/docker.sock: connect: permission denied.
See 'docker run --help'.

[INFO] Restored changes from /home/tcysin/.cache/pre-commit/patch1734636928-95070.

I'm afraid, I don't know how to make the hook run Docker with sudo 😞

@boring-cyborg boring-cyborg bot added the documentation Improvements or additions to documentation label Dec 19, 2024
@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 19, 2024
@github-actions github-actions bot added feature New feature or functionality and removed documentation Improvements or additions to documentation labels Dec 19, 2024
Copy link
Contributor

@leandrodamascena leandrodamascena left a comment

Choose a reason for hiding this comment

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

Hey @tcysin! I made some changes to this PR and let me explain those:

1 - I simplified the condition where you check self.deprecated.
2 - I understand where you're going with the change in middlewares merge method (and the defaultdict definition) and that's really cool, but I don't know if we'll have any side effects if we don't define self._routes_with_middleware[route_key] = [].
3 - I added a new test just to make sure we are not serializing the None value.
4 - I added the line you suggested in the documentation.

Again, thanks a lot for working on this PR!!

APPROVED!

aws_lambda_powertools/event_handler/api_gateway.py Outdated Show resolved Hide resolved
@leandrodamascena
Copy link
Contributor

I'm afraid, I don't know how to make the hook run Docker with sudo 😞

Hmm the .pre-commit-config.yaml is not to flexible to add sudo in there. What about if you add your current user in the docker group permission? this works?

@leandrodamascena
Copy link
Contributor

Hey @anafalcao, please approve this PR.

Copy link
Collaborator

@anafalcao anafalcao left a comment

Choose a reason for hiding this comment

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

Great! Approving this PR

@anafalcao anafalcao merged commit 4c6d196 into aws-powertools:develop Dec 19, 2024
18 checks passed
Copy link

boring-cyborg bot commented Dec 19, 2024

Awesome work, congrats on your first merged pull request and thank you for helping improve everyone's experience!

@tcysin
Copy link
Contributor Author

tcysin commented Dec 20, 2024

Hmm the .pre-commit-config.yaml is not to flexible to add sudo in there. What about if you add your current user in the docker group permission? this works?

This works 👍


Once again, thanks for your help!

@tcysin tcysin deleted the feat/mark-api-operation-as-deprecated branch December 20, 2024 19:29
@tcysin
Copy link
Contributor Author

tcysin commented Dec 20, 2024

🎉 It's alive as of 3.4.0!

Code

from aws_lambda_powertools.event_handler import APIGatewayRestResolver
from aws_lambda_powertools.event_handler.api_gateway import Router

app = APIGatewayRestResolver()

@app.get("/spam")
def spam():
    return

@app.get("/eggs", deprecated=True)
def eggs():
    return

router = Router()

@router.get("/ham", deprecated=True)
def ham():
    return

app.include_router(router, "/cheese")

if __name__ == "__main__":
    schema = app.get_openapi_json_schema()
    print(schema)

Generated JSON schema

openapi3_1.json (3.0 KB)

Swagger Editor screenshot

@leandrodamascena
Copy link
Contributor

🎉 It's alive as of 3.4.0!

Code

from aws_lambda_powertools.event_handler import APIGatewayRestResolver

from aws_lambda_powertools.event_handler.api_gateway import Router



app = APIGatewayRestResolver()



@app.get("/spam")

def spam():

    return



@app.get("/eggs", deprecated=True)

def eggs():

    return



router = Router()



@router.get("/ham", deprecated=True)

def ham():

    return



app.include_router(router, "/cheese")



if __name__ == "__main__":

    schema = app.get_openapi_json_schema()

    print(schema)

Generated JSON schema

openapi3_1.json (3.0 KB)

Swagger Editor screenshot

Thats amazing @tcysin! Nice work 👏👏👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
event_handlers feature New feature or functionality metrics size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: Mark API operation as deprecated for OpenAPI documentation
3 participants