#Code Reviews

1 messages ยท Page 2 of 1

woven tundra
#

That took a while. But I finally found it ๐Ÿ™‚

cloud helmBOT
woven tundra
#

And another one to make our glorious tooling happy ๐Ÿ™‚

woven tundra
cloud helmBOT
severe onyx
#

build docs

woven tundra
#

Yeah

severe onyx
#

im so lost in github

woven tundra
#

Love the commit history ๐Ÿ˜„

Screenshot_from_2024-01-30_16-56-33.png
severe onyx
#

๐Ÿ™‚ all what I advocate not to do at work, but it is squashed is it ?

woven tundra
signal linden
cloud helmBOT
severe onyx
snow sapphire
cloud helmBOT
woven tundra
cloud helmBOT
woven tundra
cloud helmBOT
snow sapphire
cloud helmBOT
snow sapphire
cloud helmBOT
snow sapphire
woven tundra
cloud helmBOT
woven tundra
cloud helmBOT
woven tundra
#

Once this is merged we can do the release

#

Actually, we need to wait until #general message is resolved

cloud helmBOT
woven tundra
#

Alright, I reverted the functionality to what it was before the patch, with the fix for the previous regression still intact

#

That should fix @severe onyx's problem but I can't say for sure yet because I haven't been able to reproduce

#

@severe onyx, if you have the time, could you check out the patch and confirm if that fixes it?

snow sapphire
cloud helmBOT
woven tundra
#

Ha! Reviewed before you even posted it ๐Ÿ˜›

snow sapphire
#

oh nice ๐Ÿ™‚

severe onyx
#

Will try asap

#

Obviously trying to strip down to a mvce I can't reproduce

severe onyx
rare merlin
rare merlin
cloud helmBOT
#
guacs
<:pull_open:882464248721182842> [litestar-org/polyfactory] Constrained unions

Description

  • Fix the parsing of the constraints from union types for Pydantic v2 modelsโ€- Before there used to be special handling of optional types, but that's not needed since optional types are also unions and so optional types should also be handled the same way as union types where we just ignore NoneType. This makes the change the same as that of FieldMeta.from_type (as changed in[ #491](https://github.com/litestar-org/polyfactory/issues/491)).
  • Fix the way the annotated types are handled when calling FieldMeta.from-type by using the actual type that is annotated rather than Annotated[type, ...]โ€- Due to the change in FieldMeta.type_args in[ #491](https://github.com/litestar-org/polyfactory/issues/491), Annotated[List[str], MinLen(10)] now returns List[str] whereas before it would have returned str as the child. This results in the children for the FieldMeta corresponding to the annotation Annotated[List[str], MinLen(10)] to be List[str] instead of str. Unwrapping the annotation to get the actual underlying type fixes this problem since now FieldMeta.type_args will return str in this case instead of List[str].

Close Issue(s)

Created ยท
rare merlin
cloud helmBOT
#
guacs
<:pull_open:882464248721182842> [litestar-org/litestar] fix: ensure importing of all click related stuff after running `rich_click_path`

Currently, the importing of LitestarExtensionGroup occurs before running rich_click_patch. This causes an issue due to rich_click_patch monkeypatching click.Group, click.Command etc. For example, consider the following code:

from click import Command, Group
from click.core import Command as CCommand
from click.core import Group as CGroup

from litestar.cli import litestar_group

print(isinstance(litestar_group, (Group, Command)))  # False
print(isinstance(litestar_group, (CGroup, CCommand)))  # True

# These fail.
assert Group is CGroup
assert Command is CCommand

The reason for this behavior is that LitestarExtensionGroup is not a subclass of RichGroup, but a subclass of click.Group if it gets imported before rich_click gets a chance to do its patching. Then by the time we do the isinstance check, click.Group is patched to become rich_click.RichGroup and so the isinstance(litestar_group, click.Group fails.

This causes issues in the documentation generation using sphinx-click which does a isinstance which is why this CI is failing in #3087.

To fix this, we simply ensure that we run the rich_click patching before we any imports from click.

Created ยท
wicked abyss
rare merlin
wicked abyss
#

let me type it out

#

it will be dumb, but here I go

# 
proc = multiprocessing.Process(target=run)
proc.start()
try:
    for _ in range(10000):
        try:
            httpx.get(f"http://127.0.0.1:{port}", timeout=0.1)
            break
        except httpx.TransportError:
            time.sleep(0.1)
    else:
        raise StartupError(f"App {path} failed to come online")
    yield port

finally:
        proc.kill()
        AVAILABLE_PORTS.append(port)
        port = _get_available_port()
        time.sleep(0.2)
#

no dont reply

#

I realized how dumb I am

#

That for loop doesn't result in the port being changed

#

thanks, yet one more use case where python needs labeled breaks

#

but we dont :/

rare merlin
#

Actually, I just realized I did something stupid. I don't actually catch the StartupError which means even though the finally block gets run, we won't actually retry....

#

Okay fixed it.

cloud helmBOT
#
guacs
<:pull_open:882464248721182842> [litestar-org/litestar] docs: retry with different ports if examples don't come online

There are a lot of cases where the examples fail to come online resulting in the building of the docs to fail as seen here. This is due to the port that is being used to run the application ends up being used by another example during sphinx's parallel build. While we get the available port before creating the app in run_app, by the time we actually create the subprocess and get uvicorn to run the application, the port might already be in use by another example.

To work around this, we simply keep on retrying with a different port until the application comes online or we exceed an upper limit on the number of retries.

Created ยท
wicked abyss
#

either way IIUC the link and the current litestar implementation are not thread / process safe

rare merlin
wicked abyss
#

pop(0) is bad any 1337coder will ask you to use deque

rare merlin
rare merlin
#

@wicked abyss do you have time to make that PR? If not, it's fine. I can do it in the evening when I'll get some free time.

wicked abyss
#

do i just run a litestar app and try to run this ?

rare merlin
#

I don't think we actually need to test it since it's not part of our main codebase or anything.

wicked abyss
wicked abyss
rare merlin
#

Oh...yeah just building the docs should work

wicked abyss
#

is it because I did something wrong with the port stuff or is my setup borked?

edit: idts, I stashed my changes, it still gets stuck at the latest main

rare merlin
wicked abyss
#

I think something is really wrong with my setup, if run pre-commit, it affects 29 files, some being rst files (none of which are files I edited)

#

I am in dumb mode today, proc.kill would be the only expression in finally right?

        except StartupError:
            time.sleep(0.2)
            count += 1
            port = _get_available_port()
        finally:
            proc.kill()
rare merlin
rare merlin
rare merlin
wicked abyss
snow sapphire
cloud helmBOT
snow sapphire
#

at some point we had some custom middleware tests that showed you could use Starlette middlewares with Litestar. However, it looks like the type signature for Starlette middleware changed towards the end of last year

#

is anyone seeing this test failure?

# 
FAILED tests/unit/test_middleware/test_middleware_handling.py::test_custom_middleware_processing[middleware1] - ValueError: too many values to unpack (expected 2)
FAILED tests/unit/test_middleware/test_middleware_handling.py::test_custom_middleware_processing[middleware2] - ValueError: too many values to unpack (expected 2)
snow sapphire
#

I'm also seeing this failure locally. It's an issue because the content type is returned as text/xml instead of application/xml

/home/oracle/Code/Starlite/litestar/tests/unit/test_template/test_template.py::test_media_type_inferred[.xml-application/xml] failed: extension = '.xml', expected_type = <MediaType.XML: 'application/xml'>
tmp_path = PosixPath('/tmp/pytest-of-oracle/pytest-161/test_media_type_inferred__xml_0')

    @pytest.mark.parametrize(
        "extension,expected_type",
        [
            (".json", MediaType.JSON),
            (".html", MediaType.HTML),
            (".html.other", MediaType.HTML),
            (".css", MediaType.CSS),
            (".xml", MediaType.XML),
            (".xml.other", MediaType.XML),
            (".txt", MediaType.TEXT),
            (".unknown", MediaType.TEXT),
            ("", MediaType.TEXT),
        ],
    )
    @pytest.mark.skipif(sys.platform == "win32", reason="mimetypes.guess_types is unreliable on windows")
    def test_media_type_inferred(extension: str, expected_type: MediaType, tmp_path: Path) -> None:
        tpl_name = f"hello{extension}"
        (tmp_path / tpl_name).write_text("hello")
    
        @get("/")
        def index() -> Template:
            return Template(template_name=tpl_name)
    
        with create_test_client(
            [index], template_config=TemplateConfig(directory=tmp_path, engine=JinjaTemplateEngine)
        ) as client:
            res = client.get("/")
            assert res.status_code == 200
>           assert res.headers["content-type"].startswith(expected_type.value)
E           AssertionError: assert False
E            +  where False = <built-in method startswith of str object at 0x7f2bb5a18da0>('application/xml')
E            +    where <built-in method startswith of str object at 0x7f2bb5a18da0> = 'text/xml; charset=utf-8'.startswith
E            +    and   'application/xml' = <MediaType.XML: 'application/xml'>.value

tests/unit/test_template/test_template.py:126: AssertionError
#

Is my local install broken or are others getting these errors too?

#

I'm guessing this to_asgi_response in Template may be guessing the wrong type?

                for suffix in suffixes:
                    if _type := guess_type(f"name{suffix}")[0]:
                        media_type = _type
                        break
                else:
                    media_type = MediaType.TEXT
#

yeah, media_type is set to text/xml instead of application/xml

#

in the standard python mimetypes module, .xml is mapped to text/xml

#

I'm not sure what a fix for this would look like? Any ideas? Adding XML_TEXT to our MediaTypes enum? Change the existing XML to text/xml?

#

For reference, here's the MediaType enum:

class MediaType(str, Enum):
    """An Enum for ``Content-Type`` header values."""

    JSON = "application/json"
    MESSAGEPACK = "application/x-msgpack"
    HTML = "text/html"
    TEXT = "text/plain"
    CSS = "text/css"
    XML = "application/xml"
#

For now, i've updated the test case so it's looking for text/xml instead.

snow sapphire
#

Well, let me walk all of this back. This appears to be a difference between python versions

hidden arrow
cloud helmBOT
hidden arrow
cloud helmBOT
#
tuukkamustonen
<:pull_open:882464248721182842> [litestar-org/litestar] feat: Support `ResponseSpec(..., examples=[...])`

Fixes #3068

Allows to define custom examples for the responses via ResponseSpec.

The examples set this way are always generated locally, for each response:

{
  "paths": {
    "/": {
      "get": {
        "responses": {
          "200": {
            "content": {
              "application/json": {
                "schema": {...},
                "examples": ...

Examples that go within the schema definition cannot be set by this.

Created ยท
woven tundra
cloud helmBOT
woven tundra
#

Fixes the currently failing platform compat tests on main

#

We should run those on develop as well so this doesn't happen ๐Ÿ‘€

severe onyx
#

I'm sorry I mabe didnt folow well the tooling changes, but I checkout either from main or develop and just add a docs stuff, run pre-commit and get like 50ish files changed, so I might be doing something silly ?

woven tundra
#

Which hook is doing the changing?

severe onyx
#

prettier

#

blacken-docs

woven tundra
#

I think there was a recent issue there to fix this

#

Are your branches up to date?

severe onyx
# 
โฏ git checkout main                                      ๎œผ 3.11.7 ๎œผ litestar at ๏€— 14:15:51
Your branch is up to date with 'origin/main'.
โฏ git branch --delete 3083
Deleted branch 3083 (was 1e42fa273).branch --delete 3083                               ๎œผ 3.11.7 ๎œผ litestar at ๏€— 14:15:53
โฏ git fetch upstream
โฏ git merge upstream/main
Already up to date.n ๏„“ ๏„ฆ main โฏ git merge upstream/main                                ๎œผ 3.11.7 ๎œผ litestar at ๏€— 14:16:07
โฏ git checkout -b 3083
Switched to a new branch '3083'
โฏ pre-commit run --all-files

i think so

woven tundra
#

Huh

#

Can you try reinstalling the pre-commit hooks?

severe onyx
#

I'll try recreate my venv anyways should code on 3.8

wicked abyss
#

at least I had to yolo and run docs on 3.11, test on 3.8

severe onyx
#

I blame the data-scientists I'm working with, they're bleeding on me

woven tundra
#

I just reinstalled pre-commit and now I'm getting a change from prettier as well. It's just one file though

#

Why do we even have that? Can't we just remove it? ๐Ÿ‘€

woven tundra
cloud helmBOT
woven tundra
#

There

#

๐Ÿ‘€

cloud helmBOT
severe onyx
#

@woven tundra just a little question, before I try this PR that would close the RedisStore automatically, is there a place in the current code I can look at to get a sense what bits I should look at or that's unchartered territory ? I just dont know well enough all the code and often looking at already existing things is for me a way to not fuck up everything or reinvent the wheel in a bad manner ๐Ÿ™‚

woven tundra
cloud helmBOT
#

litestar/channels/plugin.py line 33

class ChannelsPlugin(InitPluginProtocol, AbstractAsyncContextManager):
woven tundra
#

It implements the async contextmanager protocol and then adds itself to the app's lifespan handlers

#

Since stores aren't really plugins though you won't have that immediately available though

#

I can see two ways of handling this:

  1. Make the stores inherit from InitPluginProtocol and have special casing in the app init that adds them to the installed plugins
  2. Add dedicated on_startup/on_shutdown methods to the plugins (or a contextmanager) which then gets special casing in the app's init where the stores are added to the app's lifespan managers
severe onyx
#

ok makes sense reading that, hard part will be to figure it out for real ๐Ÿ™‚

woven tundra
#

Lmk if something seems to cumbersome. Maybe we'll need to come up with a deeper fix somewhere else then

severe onyx
#

well I'll try and see if that is above my league I'll let you know ! I know I'm pretty bad usually on abstractions so I take it as a challenge lol

woven tundra
cloud helmBOT
severe onyx
woven tundra
#

I left 2 comments that should help fix the issues! ๐Ÿ™‚

woven tundra
cloud helmBOT
#
provinzkraut
<:pull_open:882464248721182842> [litestar-org/litestar] fix(DTO): DTO msgspec meta constraints not being included in transfer model

Fix an issue where msgspec constraints set in msgspec.Meta would not be honoured by the DTO.

In the given example, the min_length=3 constraint would be ignored by the model generated by MsgspecDTO.

from typing import Annotated

import msgspec
from litestar import post, Litestar
from litestar.dto import MsgspecDTO

class Request(msgspec.Struct):
    foo: Annotated[str, msgspec.Meta(min_length=3)]

@post("/example/", dto=MsgspecDTO[Request])
async def example(data: Request) -> Request:
    return data

Constraints like these are now transferred.

Two things to note are:

  • For DTOs with DTOConfig(partial=True) we cannot transfer the length constraints as they are only supported on fields that as subtypes of str, bytes or a collection type, but partial=True sets all fields as T | UNSET
  • For the PiccoloDTO, fields which are not required will also drop the length constraints. A warning about this will be raised here.

Closes #3026

Created ยท
severe onyx
#

just to understand the big-picture of the test suite, especially the redis side of things, it uses docker compose if I get it corretly ? I'm not very familiar with github, but it doesn't seem to use what would be the gilab equivalent ie services (https://docs.gitlab.com/ee/ci/services/redis.html). Just to understand how to bring it alive in a test

severe onyx
#

ok this is it, will come up with "fixing" the tests, imho there is some inconsitency in the redis_client fixture

wicked abyss
severe onyx
#

Well, I may have been too fast in saying that, I keep posted

#

in this fixture a sync redis instance is created, it flushall, but it is never closed

#

so I put the flushall in the finally and just uses the async instance

#

makes more sense to me, this keeps the test suite intact I think

wicked abyss
#

ohh you mean in the fixture itself, I thought you just had issues with the stores test, when I modified the tests the fixture was already there

woven tundra
#

Also, reason it has a flushall() in the beginning is because that means we don't have to rely on the previous test having completed its cleanup properly

severe onyx
#

I can try a PR with that single change to test if you want, it ends up being the same imho, the 1st time you get the redis_client fixture you ave a clean redis instance anyways, so I think forcing it in the finally ends up doing the same, while not having a dangling sync Redis client

woven tundra
#

I'm thinking more what if one test crashes and doesn't do the cleanup? That could then affect other tests. Not a super big issue, but can make it a bit more cumbersome to debug

severe onyx
#

I'm not clear enough but shouldn't the finally make it a garantee ?

#

I mean flushall in the finally

woven tundra
#

Ah

#

I see the confusion now

#

The finally clause does not guard the yield

#

It only guards the await client.aclose()

#

So the current logic calls client.flushall() after the client has been close, regardless of whether that operation succeeded

#

The reason why the await client.aclose() is within a try/except clause to begin with though was that there had been some funky stuff going on with the async Redis client regarding that close() call, where it would sometimes manage to schedule an operation that would run after the event loop had already been closed, due to it not cleaning up all resources properly (iirc)

#

I haven't checked this issue in a while so it might actually have been fixed in the meantime and we can get rid of the whole exception handling shennanigans there and just do

yield client
await client.flushall()
await client.aclose()
severe onyx
#

yep, agree, will try in a seperate PR, also I pointed out there is one test where with_client and passing the fixture are used in the same test and I think that is an issue because the with_client one doesnt close

#

with_client not patched I should add

woven tundra
woven tundra
#

Ah, gotcha

#

Yeah, I agree. That should probably also be mocked. Or it could just use the store as a context manager and ensure that it closes properly that way

#

I think I'd even prefer that because it uses less mocking

woven tundra
cloud helmBOT
wicked abyss
cloud helmBOT
snow sapphire
cloud helmBOT
snow sapphire
#

Too slow. Thanks @woven tundra

woven tundra
#

I was faster ๐Ÿ˜›

snow sapphire
cloud helmBOT
jagged plaza
cloud helmBOT
wicked abyss
jagged plaza
#

But clients have request_class param already.

wicked abyss
#

ohh

#

sorry, I must sleep

jagged plaza
#

good night!

jagged plaza
#

Do you think that exposing websocket_class to other layers could be beneficial? I could take care of this.

snow sapphire
#

Is it currently exposed at any layer?

jagged plaza
#

Only app

#

I would take care of it.

rare merlin
cloud helmBOT
# rare merlin https://github.com/litestar-org/litestar/pull/3131
codecov[bot]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 98.26%. Comparing base (d109098) to head (e6188ff).
Report is 2 commits behind head on main.

@@           Coverage Diff           @@
##             main    #3131   +/-   ##
=======================================
  Coverage   98.26%   98.26%           
=======================================
  Files         320      320           
  Lines       14401    14404    +3     
  Branches     2316     2317    +1     
=======================================
+ Hits        14151    14154    +3     
  Misses        109      109           
  Partials      141      141

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Comment on litestar-org/litestar#3131 ยท
signal linden
#

that's kinda cool

rare merlin
signal linden
#

The preview showing the codecov I think because it was the last comment on that PR at the time

#

But the preview looks cool

rare merlin
#

Oh no that was because the link was for that specific comment. I edited the link to make it the same as the PR after that.

jagged plaza
cloud helmBOT
rare merlin
nocturne wagon
cloud helmBOT
severe onyx
cloud helmBOT
#
euri10
<:pull_open:882464248721182842> [litestar-org/litestar] fix: close RedisStore client in case the with_client classmethod is used

Description

WIP, it doesn't pass the test I added from the mvce in the issue (this one could certainly be simplified) so my impleentation has obviously an issue.
I made the RedisStore an async context manager, I can see the _on_shutdon method being hit, but the _redis client is still not closed or it doesnt exists.

from your comment here:
#1152626430295933060 message

Closes

Fixes #3083

Created ยท
true flame
#

hey all! Have a relatively simple PR that allows examples for the request body.

https://github.com/litestar-org/litestar/pull/3128

Wondering if that's going in the right direction/applicable. Building a tool that uses litestar and its been awesome! Allowing examples for users will let me reach feature parity w/FastAPI and use litestar going forward ๐Ÿ‘ Thanks!

cloud helmBOT
woven tundra
cloud helmBOT
#
provinzkraut
<:pull_open:882464248721182842> [litestar-org/litestar] fix(pydantic): OpenAPI schema generation for Pydantic private attributes (#3150)

Fix a bug that caused a NameError when trying to resolve forward references in Pydantic private fields.

Although we respected private fields in Pydantic and didn't include them in the schema, we still tried to extract their type annotation. This was fixed by not relying on typing.get_type_hints to get the type information, but instead using Pydantic's own APIs, allowing us to only extract information about the types of fields we're interested in.

Fixes #3150.

Created ยท
jagged plaza
cloud helmBOT
woven tundra
rare merlin
cloud helmBOT
#
guacs
<:pull_open:882464248721182842> [litestar-org/polyfactory] fix: constrained unions

Description

  • Fix the parsing of the constraints from union types for Pydantic v2 modelsโ€- Before there used to be special handling of optional types, but that's not needed since optional types are also unions and so optional types should also be handled the same way as union types where we just ignore NoneType. This makes the change the same as that of FieldMeta.from_type (as changed in[ #491](https://github.com/litestar-org/polyfactory/issues/491)).
  • Fix the way the annotated types are handled when calling FieldMeta.from-type by using the actual type that is annotated rather than Annotated[type, ...]โ€- Due to the change in FieldMeta.type_args in[ #491](https://github.com/litestar-org/polyfactory/issues/491), Annotated[List[str], MinLen(10)] now returns List[str] whereas before it would have returned str as the child. This results in the children for the FieldMeta corresponding to the annotation Annotated[List[str], MinLen(10)] to be List[str] instead of str. Unwrapping the annotation to get the actual underlying type fixes this problem since now FieldMeta.type_args will return str in this case instead of List[str].

Close Issue(s)

Created ยท
rare merlin
cloud helmBOT
#
guacs
<:pull_open:882464248721182842> [litestar-org/polyfactory] refactor: change order of imports for pydantic v1

Description

When importing PyObject from pydantic, if it's v2 then a warning is raised due to it being deprecated in favor of ImportString. The expected ImportError doesn't occur until after the import of PyObject takes place hence any user that's using pydantic v2 would see the warning. This ensures that PyObject is imported last so that the warning is not raised.

Created ยท
jagged plaza
#

Some small fix to keep consistency with type_encoders which are initialized directly in the layers.
https://github.com/litestar-org/litestar/pull/3153/
Wasn't sure where to put test module. response package is wrong IMO but the type_encoders tests are there.
Also not so sure about the fix: prefix.
Tell me what to do.

cloud helmBOT
woven tundra
#

You're on a roll ๐Ÿ˜„

#

Also not so sure about the fix: prefix.
It should be a feature and go into develop!

jagged plaza
#

Sir yes sir!

#

Will be fixed.

cloud helmBOT
woven tundra
cloud helmBOT
jagged plaza
cloud helmBOT
jagged plaza
cloud helmBOT
wicked abyss
signal linden
formal drumBOT
wicked abyss
jagged plaza
#

I will investigate it later. Based on mypy errors I wrote a script to refactor ignored lines.

jagged plaza
#

From the other hand putting type checkers comments will make the examples code less readable.

jagged plaza
#

https://github.com/litestar-org/litestar/pull/3164 Fixed time should handle this baby -> https://github.com/litestar-org/litestar/issues/1258

GitHub
test: Use time_machine in time sensitive rate limit middleware test...

Description

Use time_machine in time sensitive rate limit middleware tests

Closes #1258

GitHub
CI Test Failure: Rate limit middleware unexpected 429 response ยท Is...

_____________________________ test_exclude_opt_key _____________________________ def test_exclude_opt_key() -> None: @get("/excluded", skip_rate_limiting=True) def handler() -> None...

severe onyx
cloud helmBOT
severe onyx
#

I was on main before.....

#

should I close this and start overr

rare merlin
cloud helmBOT
rare merlin
severe onyx
rare merlin
#

Btw, @severe onyx on a completely unrelated note, is your profile picture the MC from Tower of God?

severe onyx
#

BAAM

rare merlin
#

Ohh it's been a long time since I've read it (the manga).

severe onyx
#

it's been a while too, I loved it so much

wicked abyss
#

I want to try sth (not saying I will fix :p)

severe onyx
wicked abyss
#

is this correct or wrong?

severe onyx
#

my got-foo solved it ๐Ÿ™‚

wicked abyss
#

I do not intend to raise a PR

wicked abyss
#

merge?

severe onyx
#

I checkout upstream/develop into develop locally and merged it into flash branch

#

dont ask me why this worked ๐Ÿ™‚

wicked abyss
#

ahh ok, I did the same except I did git rebase develop

woven tundra
#

Needs a rebase ๐Ÿ˜›

signal linden
#

@severe onyx i added another pr to that again, feel free to ignore though. Just wanted to get API docs in

cloud helmBOT
severe onyx
#

Ok I'll check on that later, it helps to get this structure though,

severe onyx
signal linden
#

they kinda were but then i squirreled and did them by accident :\

severe onyx
#

honestly it's way better than what I would have done ๐Ÿ™‚

#

love the "Oh no! I've been flashed!"

jagged plaza
#

I've made initial draft for https://github.com/litestar-org/litestar/issues/3147 here https://github.com/litestar-org/litestar/pull/3171/files
I have few questions:

  • I pass additional parameters to V1 dict too . It was not mentioned in the issue but it seems to be appropriate to support both pydantic versions, right?
  • I don't use mode it does not make sense for me. Do we agree?
  • What about round_trip and warnings_parameters? Only V2 supports them and frankly speaking and don't see a point support them too. What do you think about it?
  • There are _model_dump and _model_dump_json functions in pydantic.__init__ They're used in examples and tests. Should I also allow to pass additional params there?
cloud helmBOT
rare merlin
cloud helmBOT
wicked abyss
rare merlin
wicked abyss
wicked abyss
rare merlin
severe onyx
cloud helmBOT
#
euri10
<:pull_open:882464248721182842> [litestar-org/litestar] fix: sending empty data in sse in js client

Description

The added app is just to manually test the fix works, I dont know how we could incorporate it in our test suite given it involves running a server and checkin if the html page yields the event fine, maybe in e2e testing ?

Created ยท
severe onyx
#

and looking at it, it seems like it is often on the channels psycopg backend

#

now I'm no expert, but briefly looking at it, it seems to me (at least vs the asyncpg backend) that the underlying connection is not closed in the shutdown, is that correct ?

#

imho this would make sense to do that, but I dont know the channels stuff:

diff --git a/litestar/channels/backends/psycopg.py b/litestar/channels/backends/psycopg.py
index 14b53bcd1..40db0f610 100644
--- a/litestar/channels/backends/psycopg.py
+++ b/litestar/channels/backends/psycopg.py
@@ -25,6 +25,7 @@ class PsycoPgChannelsBackend(ChannelsBackend):
         await self._exit_stack.enter_async_context(self._listener_conn)
 
     async def on_shutdown(self) -> None:
+        await self._listener_conn.close()
         await self._exit_stack.aclose()
 
     async def publish(self, data: bytes, channels: Iterable[str]) -> None:
~
~
cloud helmBOT
severe onyx
#

on a side note, a bot runnning through all actions to detect failed tests and make some sort of summary, especially those marked as flaky (which is often to me a patch to "avoid" fixing lifetime ressources issues) would be very helpful

copper widget
umbral furnace
#

Hello ๐Ÿ‘‹

I worked on https://github.com/litestar-org/litestar/compare/main...jderrien:litestar:logging-queue-listener-handler?expand=1 to fix the queue_listener handler for Python 3.12, but I have also tried to fix some tests, then I had to refactor logging config... and I finally added a logging_module property to LoggingConfig (which might break some setups).

I guess it became a bit biggy and maybe I should break up this in multiple commits and PR... what do you think?

(Posted in #general this morning, I'm already working on a smaller one... anyway any feedback welcome)

GitHub
Comparing litestar-org:main...jderrien:logging-queue-listener-handl...

Production-ready, Light, Flexible and Extensible ASGI API framework | Effortlessly Build Performant APIs - Comparing litestar-org:main...jderrien:logging-queue-listener-handler ยท litestar-org/litestar

jagged plaza
cloud helmBOT
jagged plaza
cloud helmBOT
severe onyx
signal linden
severe onyx
snow sapphire
cloud helmBOT
snow sapphire
cloud helmBOT
dusky lance
snow sapphire
signal linden
#

auto merge just bit me/us ๐Ÿ˜ฆ

#

Will need to revert, that PR needed rebasing it looks like. It brought over a bunch of commits

rare merlin
cloud helmBOT
wicked abyss
rare merlin
#

Manually ๐Ÿ˜›

#

I think we will have to setup playwright or something as @woven tundra had mentioned before for the openapi portion. There tends to be a lot of issues related to the rendering (which we don't test) that we face.

wicked abyss
rare merlin
wicked abyss
wicked abyss
rare merlin
wicked abyss
rare merlin
#

@wicked abyss did you get a chance to manually test that it works? I did test it, but it wouldn't hurt for one more person to test ๐Ÿ˜›

wicked abyss
#

schema/rapidoc vs schema/rapidoc/ right? if so, that works

snow sapphire
cloud helmBOT
snow sapphire
#

I'm curious why we'd roll this back

#

I added the wrong stack trace from the user to the issue

#

here's the correct one

#

And it does look like a msgspec error. I also have reproduced the error in the test cases

#

@dusky lance @woven tundra

signal linden
snow sapphire
#

why?

signal linden
#

it looks like it merged in a bunch of unrelated commits and so needs a rebase

snow sapphire
#

ok, I don't know if i have time to do that until this evening

#

Why move it to _utils?

#

it's only called once during launch. We also may want to expose this as a CLI option soon

signal linden
#

thats just what all the others seemed to do and then use that in the other files

dusky lance
# snow sapphire I'm curious why we'd roll this back

I rolled it back because it looked like it needed more thought based on the TB that was in the issue. Granted that was the incorrect one, but I didn't have that info at the time. Also, as @woven tundra mentioned, msgspec should handle Enums natively, and given that I'd like to see a reproducer so I can fully understand the issue. Its my bad for not questioning these things upfront - but even after reading the thread in #1064114019373432912 I don't know how to reproduce that original issue.

snow sapphire
#

I linked the wrong stack trace.

#

Iโ€™ve updated it on the GitHub PR. Sorry for the confusion. the tests I added do reproduce it.

#

I donโ€™t know how it happened in-application though.

dusky lance
#

yeh that's the thing I'd hope to understand - as its coming out of serialization of the openapi schema, can we create a repro that fails in the same way - I might be able to find some time today to try

hidden arrow
#

I got a problem in https://github.com/litestar-org/litestar/pull/3204 - the test I added fails on Python 3.9 and I don't see the issue. I tried reproducing locally on the same Python version, but it passes on my machine. Fails to:

        with create_test_client([handler]) as client:
            response = client.get("/schema/openapi.json")
>           assert response.status_code == 200, response.text
E           AssertionError: 
E           assert 404 == 200
E            +  where 404 = <Response [404 Not Found]>.status_code

https://github.com/litestar-org/litestar/actions/runs/8287151261/job/22678795367?pr=3204

First it failed on Python 3.11, then after splitting one test into two it now passes on 3.11 but fails on 3.9. Uh... anyone mind taking a look?

cloud helmBOT
#
tuukkamustonen
<:pull_draft:882464249065136138> [litestar-org/litestar] feat: Support `schema_extra` in `Parameter` and `Body`

Description

This adds sort of a backdoor for modifying the generated OpenAPI spec.

The value is given as dict[str, Any] where the key must match with the
keyword parameter name in Schema.

The values are added at main level, without recursive merging (because we're
adjusting Schema object and not a dictionary). Recursive merge would be much
more work.

Chose not to implement the same for ResponseSpec because response models are
generated as schema components, while ResponseSpec can be locally different.
Handling the logic of creating new components when schema_extra is passed in
ResponseSpec would be extra effort, and isn't probably as important as being
able to adjust the inbound parameters, which are actually validated (and for
which the documentation is even more important, than for the response).

Closes

#3022

Created ยท
jagged plaza
cloud helmBOT
jagged plaza
cloud helmBOT
wicked abyss
umbral furnace
wicked abyss
wicked abyss
umbral furnace
woven tundra
cloud helmBOT
woven tundra
#

CI against develop currently failing because of this ๐Ÿ‘€

woven tundra
cloud helmBOT
snow sapphire
cloud helmBOT
# snow sapphire re: this: https://github.com/litestar-org/litestar/pull/3220#discussion_r1527402...
peterschutt

I think this class isn't being documented in the advanced-alchemy docs b/c it isn't referenced in __all__ in the advanced_alchemy.config.common module. Is that intentional? If that was added there I think that would make the class available via intersphinx and this ignore not necessary. (a lot of "i think"'s here because often what I think would fix these docs builds doesn't)..

Comment on litestar-org/litestar#3220 ยท
snow sapphire
#

Should classes that aren't directly used by the user be added to the mapping?

snow sapphire
dusky lance
#

Yes, ofc.

rare merlin
cloud helmBOT
hidden arrow
#

Update: Got around this. The same doesn't pop up in CI ๐Ÿคทโ€โ™‚๏ธ

I'm getting Mypy errors on latest main:

=> Running mypy
tests/e2e/test_pydantic.py:11: error: Function is missing a type annotation  [no-untyped-def]
tests/unit/test_contrib/test_pydantic/test_schema_plugin.py:73: error: Function is missing a type annotation  [no-untyped-def]
Found 2 errors in 2 files (checked 691 source files)

Is it just me?

jagged plaza
cloud helmBOT
wicked abyss
jagged plaza
#

Nope.

wicked abyss
#

nvm

jagged plaza
hidden arrow
snow sapphire
#

@signal linden @woven tundra

snow sapphire
#

Do we have a draft PR for 3.0 in thew works?

signal linden
#

not a PR but abranch

#

it doesnt have any changes in it yet or didnt at the time

rare merlin
cloud helmBOT
#
guacs
<:pull_open:882464248721182842> [litestar-org/litestar] docs: document guards behavior when placed at controller and app level

Description

This PR documents the behavior of guards placed at the controller and app level being executed for OPTIONS requests.

I don't know if linking to the GitHub issue is the best way to go about this. I'm open to any ideas on how to document this behavior better.

Closes

This is related to #2314, but doesn't fix the underlying issue. Instead it documents the issue as well as provides a link to the issue where the users can find a workaround for it.

EDIT: Reword to make sure GitHub doesn't consider the linked issue as being fixed by this PR.

Created ยท
umbral furnace
#

I updated https://github.com/litestar-org/litestar/pull/3185 after the comment from @woven tundra

TL;DR

  • Default logging doesn't work on the main branch for Python >= 3.12 and there is a breaking change on litestar.logging.standard.QueueListenerHandler (the constructor signature is not the same anymore)
  • With the last commit, this PR will fix the logging issue, revert the breaking change on QueueListenerHandler, and document its incompatibility with Python >= 3.12 due to the logging.config.dictConfig() update

Doc preview: https://litestar-org.github.io/litestar-docs-preview/3185/reference/logging/standard.html

I still have a typing issue https://github.com/litestar-org/litestar/pull/3185#discussion_r1529323696 :/

cloud helmBOT
dusky lance
umbral furnace
cloud helmBOT
#
jderrien
<:pull_open:882464248721182842> [litestar-org/litestar] fix(logging): queue_listener handler for Python >= 3.12

Description

  • Fix the queue_listener handler for Python 3.12

Python 3.12 introduced a new way to configure QueueHandler and QueueListener via logging.config.dictConfig(). This described here.

The listener still needs to be started & stopped, as previously. To do so, I've introduced LoggingQueueListener.

And as stated in the doc:

Any custom queue handler and listener classes will need to be defined with the same initialization signatures as QueueHandler and QueueListener.

Closes

Closes #2954

Created ยท
signal linden
cloud helmBOT
jagged plaza
cloud helmBOT
dusky lance
peak monolith
dusky lance
wicked abyss
cloud helmBOT
dusky lance
cloud helmBOT
signal linden
cloud helmBOT
snow sapphire
dusky lance
cloud helmBOT
#
peterschutt
<:pull_open:882464248721182842> [litestar-org/litestar] ci: run slotscheck cli as ci job

Description

This PR moves running slotscheck from within pre-commit to its own job in ci (same as we've done with other tools that require the configured environment to run).

This partially fixes an issue identified in #3251 where the litestar package was inadvertently excluded from this check due to an incorrect regex pattern.

Applies fixes for issues identified by the tool.

Co-authored-by: ariebovenberg [email protected]

Created ยท
rare merlin
dusky lance
woven tundra
#

Ha! Joke's on you, I was already reviewing it! ๐Ÿ˜›

dusky lance
#

foiled again!

woven tundra
#

@snow sapphire @dusky lance I've addressed some of the low hanging fruits wrt the import time

peak monolith
woven tundra
cloud helmBOT
#
provinzkraut
<:pull_open:882464248721182842> [litestar-org/litestar] feat: Warn about ambiguous default values in parameter specifications

As discussed in https://github.com/litestar-org/litestar/pull/3280#issuecomment-2026878325, we want to warn about, and eventually disallow specifying parameter defaults in two places.

To achieve this, 2 warnings are added:

  • A deprecation warning if a default is specified when using Annotated: param: Annotated[int, Parameter(..., default=1)] instead of param: Annotated[int, Parameter(...)] = 1
  • An additional warning in the above case if two default values are specified which do not match in value: param: Annotated[int, Parameter(..., default=1)] = 2

In a future version, the first one should result in an exception at startup, preventing both of these scenarios.

Created ยท
woven tundra
cloud helmBOT
#
provinzkraut
<:pull_open:882464248721182842> [litestar-org/litestar] fix: Custom types cause serialisation error in exception response with non-JSON media-type

When using a non-JSON media type (e.g. text/plain), ValidationExceptions wouldn't get serialised properly because they would ignore custom type_encoders.

The example below would result in a SerializationException:

class Foo:
    def __init__(self, value: str) -> None:
        self.value = value

@get(media_type=MediaType.TEXT)
def handler() -> None:
    raise ValidationException(extra={"foo": Foo("bar")})

app = Litestar([handler], type_encoders={Foo: lambda f: f.value})
Created ยท
snow sapphire
#

Also, why would we favor the SA type over what the user has in the impl type?. NM - I see the linked issue.

#

I think that this may have some odd side effects

peak monolith
snow sapphire
peak monolith
snow sapphire
cloud helmBOT
#

lib/sqlalchemy/sql/type_api.py line 607

def python_type(self) -> Type[Any]:
snow sapphire
#

Which raises NotImplemented by default

cloud helmBOT
#

lib/sqlalchemy/sql/type_api.py line 1108

class ExternalType(TypeEngineMixin):
snow sapphire
#

So, in that case, nice work @peak monolith. Sorry for coming in after the fact on the review.

peak monolith
#

No problem. Good to clarify and confirm! I find some of the types (hierarchies) in SQLA a bit confusing

snow sapphire
woven tundra
cloud helmBOT
#
provinzkraut
<:pull_open:882464248721182842> [litestar-org/litestar] fix(OpenAPI): Ensure default values are always represented in schema for dataclasses and `msgspec.Struct`s

Fix a bug that would prevent default values for dataclasses and msgspec.Struct`s to be included in the OpenAPI schema.

@dataclass
class SomeModel:
    field_a: str = "default_a"
    field_b: str = dataclasses.field(default="default_b")

and

class SomeModel(msgspec.Struct, kw_only=True):
    field_a: str = "default_a"
    field_b: str = msgspec.field(default="default_b")

now generate correct schemas in all cases.

Fix #3201.

Created ยท
woven tundra
cloud helmBOT
#
provinzkraut
<:pull_open:882464248721182842> [litestar-org/litestar] fix(DTO): Pydantic v2 error handling/serialization when for non-pydantic exceptions

Fix a bug that would cause a TypeError when non-Pydantic errors are raised during Pydantic's validation process while using DTOs.

This is the case for example when using field_validator:

class User(BaseModel):
    user_id: int

    @field_validator("user_id")
    @classmethod
    def validate_user_id(cls, user_id: int) -> None:
        raise ValueError("user id must be greater than 0")

The exception on our side occurs because Pydantic v2 includes the raw error objects in their error report, which we in turn use to generate our error responses.

I've added a simple fix that stringifies the errors here. This does not address the root issue though, which is how we currently handle the different ways libraries throw errors at us. We should aim for a more permanent fix, but this is unfortunately not possible without introducing breaking changes in how Litestar reports errors and therefore must be shelved until v3.

Closes #2365.

Created ยท
woven tundra
cloud helmBOT
woven tundra
cloud helmBOT
woven tundra
cloud helmBOT
#
provinzkraut
<:pull_open:882464248721182842> [litestar-org/litestar] fix(OpenAPI): Fix OpenAPI schema generation for paths with path parameters of different types on the same path

Fix a bug that would cause no OpenAPI schema to be generated for paths with path parameters that only differ on the path parameter type.

Essentially, /{param:int} and /{param:str} (that is, two paths with the same path parameter name but a different path parmeter type) could not coexist in the same application. Defining them as /{param:int} and /{other_param:str} or /{param:int} and /{param:int} (i.e. different names or same type) would result in the correct behaviour.

Closes #2700.

This was caused by the way Litestar handles routing of these; A single route can have multiple route handlers, but requires that all path parameters, if existent, be of the same type. Litestar can also handle routing with parameters of different types, but creates separate routes for this. Since this representation is only internal (for OpenAPI, they are still the same path), one OpenAPI path generated for one such route would be overwritten by the next route for that path.

Created ยท
woven tundra
cloud helmBOT
dusky lance
cloud helmBOT
#
peterschutt
<:pull_open:882464248721182842> [litestar-org/litestar] feat: Pydantic dto non instantiable types

Description

This PR simplifies the type that we apply to transfer models for pydantic field types in specific circumstances.

  • JsonValue: this field type is an instance of TypeAliasType at runtime, and contains recursive type definitions. Pydantic allow list, dict, str, bool, int, float and None, and the value types of list and dict are allowed to be the same. We type this as Any on the transfer models as this is basically the same thing for msgspec (ref).
  • EmailStr. These are typed as EmailStr which is a class at runtime which is not a str, however they are a string and if TYPE_CHECKING is used to get type checkers to play along. If we return a str from a msgspec decode hook for the type, msgspec fails input validation. So we must tell msgspec its a string. This also works well with encoding because it is one.
  • IP/Network/Interface types. These are represented by types such as IPvAnyAddress, but they are actually parsed into instances of stdlib types such as IPv4Address and others from the ipaddress module. Given that an instance of IPvAnyAddress cannot be meaningfully returned from a decoding hook, we have to tell msgspec that these are strings on the transfer models and let pydantic do the disambiguation. Encoding the stdlib ip types to str is natively handled by msgspec.

There are other types that can adequately be handled by defining type encoders/decoders for them, so I've left them as out of scope for these changes.

Also the Json type is tricky as it accepts a string and is parsed into builtin types by pydantic, however by default pydantic doesn't serialize that back into a string unless explicitly asked to do a round trip. For the DTO transfer models, this means we'd probably have to type it as str, but then we don't have a way to encode the decoded values back into a str b/c msgspec won't call an encoding hook for builtin types. So I've left this out of scope.

Closes

Closes #2334

Created ยท
dusky lance
cloud helmBOT
snow sapphire
cloud helmBOT
dusky lance
cloud helmBOT
dusky lance
#

It is based on the openapi one b/c it also modifies the what's new document which doesn't yet exist in v3 branch

snow sapphire
jagged plaza
cloud helmBOT
true flame
snow sapphire
true flame
woven tundra
#

Yeah. Same thing just for version 3.0

#

You should be able to rebase your branch onto develop and then change the branch in GH to v3.0 (hopefully) without conflicts

true flame
woven tundra
#

I'd recommend rebasing against the develop in your own fork

#

Given that it's up-to-date of course ๐Ÿ˜‰

true flame
woven tundra
#

Uh

#

Not sure, never had this warning ๐Ÿ‘€

#

You should be able to just do git rebase develop if you're on the schema-examples branch

#

What tool are you using here?

true flame
#

I'm using Pycharm, I can just run the commands tho and see what happens ๐Ÿ˜…

snow sapphire
#

git rebase --pray

snow sapphire
cloud helmBOT
#
rseeley
<:pull_open:882464248721182842> [jolt-org/advanced-alchemy] fix: accept `None` in `model_from_dict` to allow setting model fieldsโ€ฆ

โ€ฆ to NULL

This change allows for sending a dict with None as values; the model will then be updated to NULL for those fields. The existing handling in model_from_dict will drop dict keys if they're set to None.

Pull Request Checklist

  • โœ… New code has 100% test coverage
  • โ—ป๏ธ (If applicable) The prose documentation has been updated to reflect the changes introduced by this PR
  • โ—ป๏ธ (If applicable) The reference documentation has been updated to reflect the changes introduced by this PR
  • โœ… Pre-Commit Checks were ran and passed
  • โ—ป๏ธ Tests were ran and passed

Description

  • Allows for sending a dict with None as values to model_from_dict; the model will then be updated to NULL for those fields. This changes the existing behavior of model_from_dict, which will drop dict keys if they're set to None, causing the record's field to be unchanged.
Created ยท
snow sapphire
#

Does exclude_none as a name make sense here?

#

basically if you pass in data as a dictionary and the key is missing, then we do nothing with the value in the model. If this exclude_none is set to False, then all missing keys are set to NULL

#

unset_is_null, maybe? exclude_unset keeps a bit of parity with Pydantic, so it's familiar

dusky lance
signal linden
signal linden
signal linden
signal linden
signal linden
signal linden
hidden arrow
cloud helmBOT
# hidden arrow I'm resurrecting https://github.com/litestar-org/litestar/pull/3225#issuecomment...
tuukkamustonen

@guacs @peterschutt @JacobCoffee Hey, sorry for taking so long here.

I'm looking into it again, and I don't see how converting the json_schema_extra: dict into Schema object could happen conveniently:

  • Can't just use setattr() because many Schema object fields are also custom types (e.g. Schema.any_of and friends)
  • Can't easily merge nested structures, because I'd have to first convert them to right type
  • Recursively merging (not sure how Pydantic does that, but I assume recursively) would require extra logic

Properly supporting it would add a lot of (unjustified) bloat. I don't think it makes sense. (All that to just get converted back to dict again.)

Do you have better suggestions? Maybe passing extra as separate argument, if it cannot live in Schema, or sub-classing Schema for this special case?

The scope of this topic is actually wider. Pydantic also supports:

I had an idea here, to only support the callable format and do something like:

    class MyBody(pydantic_v2.BaseModel):
        def _add_examples(self, schema):
            schema.examples = [{"foo": "hello"}]
        model_config = pydantic_v2.ConfigDict(json_schema_extra=_add_examples)
        foo: str

...but Pydantic defines json_schema_extra as:

JsonSchemaExtraCallable: TypeAlias = Union[
    Callable[[JsonDict], None],
    Callable[[JsonDict, Type[Any]], None],
]

class ConfigDict(TypedDict):
    json_schema_extra: JsonDict | JsonSchemaExtraCallable | None

So we would be a bit breaking that API as in this case typing would be for Callable[[Schema], None] (and not JsonDict)... maybe not too bad, but still.

Finally, that WithJsonSchema is something Litestar doesn't support at all now (AFAIK).

I'm still wondering about the rationale to re-write the whole OpenAPI spec parsing when you could just call BaseModel.model_json_schema and let Pydantic (and MsgSpec, et al) generate it. It would automatically support the bells and whistles seen here, plus any that they might add/alter in the future. And you'd save a lot of extra code, rather than trying to play the catch up game?

Comment on litestar-org/litestar#3225 ยท
peterschutt

I find this a little confusing:

I think that would (should) be considered a hack and is definitely not an intentional public API for this ๐Ÿ‘€

It is in contrast to the reality of what we actually do. We pull anything out of the metadata for a type that we parse and add it to a ParameterDefinition instance for the type as long as it has the right attributes:

https://github.com/litestar-org/litestar/blob/6e7e54ad856aa31bcbaf908a7349a7aa4a30808f/litestar/typing.py#L104-L130

In general, Litestar doesn't try to enhance the support of the modelling library you chose beyond what it offers out of the box.

Yet we have a unit test checking that annotated-types is supported on a dataclass definition for schema generation:

https://github.com/litestar-org/litestar/blob/6e7e54ad856aa31bcbaf908a7349a7aa4a30808f/tests/unit/test_openapi/test_schema.py#L300-L329

So for the following application:

from dataclasses import dataclass

import annotated_types
from typing_extensions import Annotated

from litestar import Litestar, post

@dataclass
class Foo:
    foo: Annotated[str, annotated_types.MaxLen(3)]

@post()
async def post_handler(data: Foo) -> None:
    ...

app = Litestar([post_handler])

We do render the constraint:

!image

.. yet it is not enforced:

!image

However, add a DTO into the mix:

@post(dto=DataclassDTO[Foo])
async def post_handler(data: Foo) -> None:
    ...

And now those constraints are enforced:

!image

This is all a by-product of the way that we indiscriminately parse anything out of type metadata if it smells like a thing that declares schema constraints, immediately upon parsing the type. And once we have the constraint parsed, the DTOs will convert it into msgspec.Meta for the transfer model.

It also opens us up to weird issues like this app failing on schema generation:

from dataclasses import dataclass

from typing_extensions import Annotated

from litestar import Litestar, post

@dataclass
class Foo:
    foo: Annotated[str, "totally unrelated metadata"]

@post()
async def post_handler(data: Foo) -> None:
    ...

app = Litestar([post_handler], debug=True)

!image

This occurs because the string "totally unrelated metadata" has a title attribute that has nothing to do with ...

Comment on litestar-org/litestar#3202 ยท
snow sapphire
jagged plaza
cloud helmBOT
#
kedod
<:pull_open:882464248721182842> [litestar-org/litestar] feat: Add `path` parameter to Litestar application class

Description

  • Expose path parameter at Litestar application class level

At this moment if we want use prefixed path for all routes in the application we have to create
dedicated router. E.g:

from litestar import Litestar, Router, get


@get("/")
async def index() -> str:
    return "Hello, world!"


router = Router(path="/path", route_handlers=[index])


app = Litestar(route_handlers=[router])

This PR allows to set path on the Litestar application level.
Thanks to this we don't need additional Router class.

from litestar import Litestar, get


@get("/")
async def index() -> str:
    return "Hello, world!"


app = Litestar(path="/path", route_handlers=[index])
Created ยท
snow sapphire
#

Nice

jagged plaza
#

I only made this bc I didn't want to fight with the elasticsearch config right now ๐Ÿ˜›

snow sapphire
dusky lance
jagged plaza
cloud helmBOT
#
kedod
<:pull_open:882464248721182842> [litestar-org/litestar.dev] docs: Update maintainers list

Pull Request Checklist

  • โ—ป๏ธ New code has 100% test coverage
  • โ—ป๏ธ (If applicable) The prose documentation has been updated to reflect the changes introduced by this PR
  • โ—ป๏ธ (If applicable) The reference documentation has been updated to reflect the changes introduced by this PR
  • โ—ป๏ธ Pre-Commit Checks were ran and passed
  • โ—ป๏ธ Tests were ran and passed

Description

  • Update maintainers list

Close Issue(s)

Created ยท
jagged plaza
#

@signal linden you are super fast with merging.

wicked abyss
jagged plaza
#

You are faster @wicked abyss ๐Ÿ˜—

signal linden
#

Alrighty

cloud helmBOT
signal linden
#

special attention pls on anything we can cross-link with intersphinx or python glossary

#

and also the summation of content because some things were quite long and had coce blocks

woven tundra
#

revieweded

signal linden
#

thx i fixededed

woven tundra
cloud helmBOT
jagged plaza
cloud helmBOT
snow sapphire
dusky lance
woven tundra
#

Looksie given

wicked abyss
rare merlin
woven tundra
cloud helmBOT
woven tundra
#

This also would like a looksie ๐Ÿ™‚

#

(is that an established slang term for "review" now? ๐Ÿ˜„)

snow sapphire
#

it's very similar to the litestar usage now

snow sapphire
#

Removes the customization in favor of the bundled AA abstractions

#

it also fixes an issue where there was a warning about the event loop being closed printed during 2 tests

#

they weren't failures but it was annoying to see in the output screen

#

it also swaps to pytest-databses instead of the built in docker fixtures

#

oh, and uses the new render_plugins for open API

jagged plaza
#

Liteatar maintainers are top models now!

snow sapphire
woven tundra
#

Do I also need a profile picture with sunglasses now?! ๐Ÿ˜„

#

Should we make that mandatory? ๐Ÿ˜›
"Maintainer requirements: Must look at least this cool with sunglasses" stickbug

snow sapphire
#

what picture?

#

lol

woven tundra
#

๐Ÿ˜„

#

Sneaky!

jagged plaza
#

That would be super cool. I guess the number od sponsors would increase rapidly...

woven tundra
#

As you wish

#

๐Ÿ˜„

snow sapphire
#

OK, new glasses pic

woven tundra
#

"code review" > "glasses review"

snow sapphire
#

I even added a banner pic

signal linden
#

done

woven tundra
#

@signal linden wins

jagged plaza
#

Vouge - "Litestar is the most sexyf and hot framework - Interview with Janek and Cofin"

woven tundra
#

Good idea. I feel like promoting a framework as "fast" has become old

jagged plaza
#

"During the night I wear only Lightstar" - Victoria "Posh" Beckham

woven tundra
jagged plaza
#

"Is FastAPI out of fashion? Check out the Litestar!"

woven tundra
#

I mean, I have a cool Litestar hoodie

dusky lance
snow sapphire
#

Done

dusky lance
#

Ta!

dusky lance
#

we'll need a point release for this too if there's anyone around who knows what they are doing on that front

wicked abyss
dusky lance
#

Well, we don't have a test environment that only has pydantic v1 in it, do we?

#

We should probably have one if not - but I'd like to get this out

snow sapphire
wicked abyss
dusky lance
#

but are we going pydantic v2 only in 3.0??

dusky lance
wicked abyss
dusky lance
#

haha - I'm actually wearing my sunnies right now b/c I was heading out the door to go for a run before I saw the issue ๐Ÿ˜Ž

#

There you go @wicked abyss

wicked abyss
dusky lance
#

I'll do a release after work this afternoon if no one beats me to it - haven't done one in a while though but I assume the instructions are all there in contributing?

wicked abyss
signal linden
#

we still havent finished the cherry-pick-release-ifier

dusky lance
#

hmm, so do I need to create a new branch from the 2.8 tag and cherry pick the patch and tag/release that as 2.8.1?

signal linden
dusky lance
cloud helmBOT
dusky lance
woven tundra
dusky lance
dusky lance
#

this passed on release of v2.8.0 - I feel like it should have failed given that bug

rare merlin
rare merlin
cloud helmBOT
#
peterschutt
<:pull_open:882464248721182842> [litestar-org/litestar] feat: problem details plugin

Description

A plugin to enable usage of problem details as the response.

The way this works is by injecting an exception handler into the app level exception handlers to convert ProblemDetailsException into a response following the specification as per RFC 9457.

Users can pass in a mapping of exception types to callables that will convert those exception types into ProblemDetailsException for handling specific exceptions such as pydantic's ValidationError. That converted ProblemDetailsException will then be used to create the response. This should allow for flexibility when needed.

Closes

Closes #3199.

Labels

area/plugins, pr/internal, size: small, type/feat

Created ยท
rare merlin
cloud helmBOT
signal linden
dusky lance
#

This needs to be reviewed, but it has exposed an issue we have when pydantic v1.10 users update to the latest of that minor series. We rely on v1 not being in the pydantic namespace to ascertain if we have v1 installed or v2 installed. This is no longer valid as they've added v1 namespace into v1

#

In that PR I've moved those minimal example/environment tests into normal CI b/c they don't take long to run and surely its better to find out about these issues well before we are about to cut a release.. right?

signal linden
signal linden
dusky lance
dusky lance
cloud helmBOT
woven tundra
cloud helmBOT
woven tundra
#

First big one for v3.0 ๐Ÿ™‚

snow sapphire
dusky lance
dusky lance
cloud helmBOT
dusky lance
#

it contains a "What's new in v3" doc which will be useful for subsequent v3 PRs

signal linden
dusky lance
#

Which PR is that?

#

Update to v3 style

#

?

signal linden
#

yea

dusky lance
#

if you get that merged I'll cherry pick the openapi controller on onto that and redo the docs part

cloud helmBOT
dusky lance
#

How do you think we should proceed with the docs one @signal linden - I notice the ci builds based off it are failing b/c of changes to depenedency groups?

dusky lance
#

Its a weird one as I don't see any changes in the pyproject.toml that should change the groups but in the lock file it drops all of the groups except "default"and "docs"

signal linden
#

err litestar-sphinx-theme#15

cloud helmBOT
dusky lance
#

Regenerating the lock helped, now I have groups = ["default", "dev", "dev-contrib", "docs", "linting", "test"]

#

but still get:

(litestar-3.8) peter@pop-os:~/PycharmProjects/litestar$ pdm install -G:all
[PdmUsageError]: Requested groups not in lockfile: full,jinja,attrs,piccolo,pydantic,redis,cli,mako,jwt,picologging,annotated-types,standard,structlog,opentelemetry,cryptography,brotli,minijinja,prometheus,sqlalchemy
(litestar-3.8) peter@pop-os:~/PycharmProjects/litestar$
#

Ahh pdm lock -G:all

#

Do you mind if I push that lockfile into that branch @signal linden ?

signal linden
#

anytime

signal linden
dusky lance
cloud helmBOT
dusky lance
jagged plaza
cloud helmBOT
#
kedod
<:pull_open:882464248721182842> [litestar-org/pytest-databases] feat: Add Elasticsearch support

Pull Request Checklist

  • โœ… New code has 100% test coverage
  • โœ… (If applicable) The prose documentation has been updated to reflect the changes introduced by this PR
  • โœ… (If applicable) The reference documentation has been updated to reflect the changes introduced by this PR
  • โœ… Pre-Commit Checks were ran and passed
  • โœ… Tests were ran and passed

Description

  • Add Elasticsearch support

TODO:

  • โœ… More tests
  • โœ… Mor accurate tests :)
  • โœ… Replace tests with async ones
Created ยท
jagged plaza
cloud helmBOT
signal linden
jagged plaza
signal linden
#

We can ignore them for now, i dont really understand which thing is failing and where ebcause it is indeed still labeling them even though it cusses at us

#

maybe the permissions are wrong for that token

#

ill try to fix it when i get through with some work

jagged plaza
#

Thanks Coffe

dusky lance
cloud helmBOT
dusky lance
woven tundra
cloud helmBOT
#
provinzkraut
<:pull_open:882464248721182842> [litestar-org/litestar] fix: Solve a caching issue in `CacheControlHeader`

I noticed this test was breaking when executed standalone or in a particular order. It turns that the test case (added in #1917) never worked in the first place and was only passing because invalid results of the _get_field_names classmethod were saved on the class.

Since the caching isn't really needed for performance here I have just removed it and fixed the test.

Labels

area/datastructures, pr/internal, size: small, type/bug

Created ยท
woven tundra
cloud helmBOT
woven tundra
#

Some deprecation work

cloud helmBOT
#
provinzkraut
<:pull_open:882464248721182842> [litestar-org/litestar] refactor(routing)!: Refactor routes and route handlers [WIP]

Refactor routes and route handlers in an attempt to simplify configuration.

The two general goals here are:

  • Routes should not be concerned with any logic pertaining to handlers
  • Routing layers (e.g. app/router instances) should not perform any manual handler setup

In general I'd like to restrict the routes to an internal part of the ASGI routing, only taking care of mapping a path > route handler(s), and establishing the connection. Everything else should be taken care of by the handlers.

What this currently does:

  • Remove the handler_names property from HTTPRoute
  • Move the OPTIONS handler creation from the HTTPRoute to the route registration process
  • Remove the methods attribute from BaseRoute; It does not make sense on the asgi/websocket routes
  • Move the creation of the KwargsModel from the routes to the handlers and remove the outside mutation of the handlers
  • Move the mapping of route > handler > kwargsmodel to the handler itself
  • Move the actual route handling to the handler
  • Establish ASGIConnection/WebSocket/Request in the routes and pass them to handlers instead of raw ASGI objects (scope, receive, send)
  • Some minor refactorings that were enabled by these things

Todo:

  • โ—ป๏ธ Add section in "what's new"
  • โ—ป๏ธ Add deprecations to the 2.x line for removed things
Labels

area/docs, area/handlers, area/openapi, area/private-api, area/router, do not merge, pr/internal, size: large

Created ยท
woven tundra
#

This is also ready to go I think

#

There's more to be done here, but I don't want to blow up the PR unnecessarily ๐Ÿ™‚

dusky lance
wicked abyss
#

shouldnt it be "Removed scope state utilities"

dusky lance
#

yes

#

the change stores the exceptions handlers in the scope state so that the handler middleware can access from there. This means that we never require the middle exception handler wrapper as the outer one can now access the exception handlers resolved from the handler.

dusky lance
#

https://github.com/litestar-org/litestar/pull/3395 - changes to our CORS handling - there's two commits in this one that maybe should be split up, so perhaps good to look at each commit individually, as the 2nd one is just something I noticed while working on the first. That is, we have a dependency on the CORSMiddleware in the ExceptionHandlerMiddleware, so that we can wrap the send() coro with the CORS middleware send wrapper if CORS is configured. So, why not just make the CORSMiddleware the outermost middleware in this case so that the send() coro received by the exception handler middleware is already the wrapped one, if CORS is configured?

cloud helmBOT
#
peterschutt
<:pull_open:882464248721182842> [litestar-org/litestar] refactor: CORS handling

Description

Applies two refactors to our CORS handling:

  • Move pre-flight response generation from the generated OPTIONS handlers to the CORS middleware.
  • Make CORS outermost middleware.

Fixes case where pre-flight response not applied to mounted ASGI apps. Previously, we would only generate OPTIONS handlers for HTTP routes, not mounts, so pre-flight requests would be passed through to the mounted ASGI app.

Fixes the case where a user defined OPTIONS handler may not include CORS logic. In fact, this is they only way we can be sure that we honor CORS pre-flight if the config object is provided.

Remove ExceptionHandlerMiddleware's dependence on CORSMiddleware by making CORSMiddleware the outermost middleware in the stack if the app has CORS configured. This makes responses sent via the exception middleware pass through the CORS middleware send wrapper if CORS is configured, which is behaviourally equal to the current behavior of wrapping the send coro inside the exception handler middleware with the CORS middleware send wrapper.

Closes

Labels

area/middleware, pr/internal, size: small

Created ยท
dusky lance
#

Also, I'm uncertain whether we should consider the first commit a breaking change. It removes our reliance on the auto-generated OPTIONS handlers for generating the pre-flight response, but it is feasible that someone is supplying a custom OPTIONS handler specifically to handle the pre-flight response. However, the counter to this is that there is no documented requirement that a custom OPTIONS handler must handle the CORS pre-flight response, which could result in pre-flight requests not being handled where there is a CORS config on the app, but a custom OPTIONS handler defined for a route. This is because we only conditionally generate the OPTIONS handler with the CORS pre-flight logic in it if there is not already an OPTIONS handler on the route.

dusky lance
#

OK I broke the CORS pr up into 3 separate ones. They all chain off eachother but in order:

  1. litestar#3400 - this changes the way we build the asgi stack when cors is enabled. Instead of wrapping the stack with ExceptionHandlerMiddleware we wrap it in CORSMiddleware which removes the need for ExceptionHandlerMiddleware to depend on CORSMiddleware.
  2. litestar#3401 - this moves the logic of generating CORS pre-flight responses from the generated OPTIONS handlers to CORSMiddleware itself. This fixes an issue where we aren't applying CORS to mounted apps.
  3. litestar#3404 - deprecate CORS middleware from our public API - its pointless being there b/c our use of it is hardcoded so it cannot be subclassed, we need it to be the outermost layer of the asgi stack and it is configured by the CORSConfig object, not by adding a middleware to the application.
cloud helmBOT
signal linden
#

Wow fancy @wicked abyss

snow sapphire
snow sapphire
snow sapphire
woven tundra
cloud helmBOT
wicked abyss
# cloud helm

why did the linter not catch the type errors before?

woven tundra
#

Because of the dynamic types I added

#

Thatโ€™s new, which prevents mypy from correctly inferring the type there

dusky lance
dusky lance
#

litestar#3418

cloud helmBOT
severe onyx
cloud helmBOT
dusky lance
#

it has been reviewed before, but materially changed since then.

#

the crux of it is that instead of storing the exception handlers resolved for each handler on the ExceptionHandlerMiddleware instances in the asgi stack, we store them in scope once routing has completed and access them from the EHMs if needed. This reduces the need for 1 instance of EHM in the stack in all cases.

#

It also adds an optimization to not add the inner-most EHM instance (that wraps route.handle) if there is no middleware in the stack.

#

Lastly, it deprecates the EHM from public API, as similar to CORS, we have specific hard-coded use of the type in our internals, so making it public doesn't make any sense b/c users cannot customize it.

woven tundra
#

This is amazing @dusky lance!

dusky lance
# woven tundra This is amazing <@947242363439448155>!

thanks! It just got a slight bit larger as I recalled that create_exception_response() and create_debug_response() are used downstream in apps like fullstack, so I've moved them into exceptions.responses real estate, and documented them.

signal linden
#

on both, establishes v3 docs for these new libs and moved them from jolt, updates license year, some housekeeping

dusky lance
#

Thanks for doing that @signal linden!

signal linden
#

np

#

idk why the builds failing but

#

almost there

cloud helmBOT
signal linden
cloud helmBOT
woven tundra
cloud helmBOT
#
provinzkraut
<:pull_open:882464248721182842> [litestar-org/litestar] feat!: Remove deprecated `litestar.middleware.exceptions` module and deprecated params of internal `ExceptionHandlerMiddleware`

Description

  • Remove deprecated module litestar.middleware.exceptions
  • Remove deprecated app param of ExceptionHandlerMiddleware
  • Remove deprecated exception_handlers param of ExceptionHandlerMiddleware

Closes

Labels

Breaking ๐Ÿ”จ, area/asgi, area/docs, area/private-api, pr/internal, size: small, type/feat

Created ยท
jagged plaza
cloud helmBOT
wicked abyss
woven tundra
cloud helmBOT
woven tundra
cloud helmBOT
#
provinzkraut
<:pull_open:882464248721182842> [litestar-org/litestar] feat: Make route handlers functional decorators

Make route handler decorators functional decorators.

This gets rids of the __call__ method having to mutate internal state and lets us clean up things a bit.

It also makes for a nicer syntax and improved type/runtime checking of the handler classes, because they can now receive a fn as a required keyword argument, which gets rid of the HandlerClass("/")(handler_fn) pattern.

Labels

area/channels, area/controller, area/docs, area/handlers, area/private-api, area/testing, area/types, pr/internal, size: large, type/feat

Created ยท
woven tundra
#

Second round of route handler refactorings

woven tundra
cloud helmBOT
wicked abyss
#

Fix about 900 warnings in our test suite when running on 3.12.

#

just dropping it

dusky lance
severe onyx
cloud helmBOT
peak monolith
cloud helmBOT
severe onyx
#

can I close 3411 in favor of 3408 ?

wicked abyss
#

but I am not 100% sure

#

not sure if Sequence | None vs NotASequence | None are different

severe onyx
woven tundra
# cloud helm

Friendly bump. <@&919261960921546815>, this needs some eyes ๐Ÿ™‚

woven tundra
dusky lance
#

Iโ€™ll have time to go over it today ๐Ÿ‘

dusky lance
severe onyx
cloud helmBOT
dusky lance
severe onyx
jagged plaza
cloud helmBOT
wicked abyss
signal linden
cloud helmBOT
rare merlin
signal linden
rare merlin
#

Hm..weird

signal linden
#

Yeah after clean i get a ton.. ones not even showing on github ci

mypy.....................................................................Failed
- hook id: mypy
- exit code: 1

polyfactory/value_generators/constrained_dates.py:41: error: Redundant cast to "date"  [redundant-cast]
polyfactory/factories/base.py:508: error: Argument 1 to "UUID" has incompatible type "bytes | str | UUID"; expected "str | None"  [arg-type]
tests/test_random_configuration.py:68: error: Redundant cast to "int"  [redundant-cast]
polyfactory/factories/pydantic_factory.py:546: error: Incompatible return value type (got "dict[Any, object]", expected "dict[Any, Callable[[], Any]]")  [return-value]
tests/test_recursive_models.py:56: error: Non-overlapping identity check (left operand type: "PydanticNode", right operand type: "type[_Sentinel]")  [comparison-overlap]
docs/examples/decorators/test_example_1.py:19: error: Returning Any from function declared to return "datetime"  [no-any-return]
docs/examples/decorators/test_example_1.py:19: error: Redundant cast to "timedelta"  [redundant-cast]
polyfactory/factories/beanie_odm_factory.py:32: error: Unused "type: ignore" comment  [unused-ignore]
Found 8 errors in 7 files (checked 129 source files)
#

If its okay i was going to merge because making any changes would be pretty out of scope

#

its probably from a mypy version bump if i had to guess

rare merlin
#

Yeah that's fine. Could you create an issue for this as well so we can fix it?

wicked abyss
rare merlin
signal linden
cloud helmBOT
signal linden
cloud helmBOT
signal linden
cloud helmBOT
snow sapphire
#

want me to wait until your resolve the conflicts to approve?

signal linden
#

aww man howd that happen

#

oh i did a bad out of scope thing that then also was changed in this pr because it bugged me

#

bad

#

fixed

snow sapphire
snow sapphire
vast yoke
cloud helmBOT
wicked abyss
snow sapphire
snow sapphire
snow sapphire
cloud helmBOT
#
guacs
<:pull_open:882464248721182842> [litestar-org/litestar-asyncpg] fix: correct the type hints for setup and init

Pull Request Checklist

  • โ—ป๏ธ New code has 100% test coverage
  • โ—ป๏ธ (If applicable) The prose documentation has been updated to reflect the changes introduced by this PR
  • โ—ป๏ธ (If applicable) The reference documentation has been updated to reflect the changes introduced by this PR
  • โœ… Pre-Commit Checks were ran and passed
  • โ—ป๏ธ Tests were ran and passed

Description

  • correct the type hint for the setup and init in PoolConfig
Created ยท
rare merlin
#

Aah because setup and init doesn't take in a coroutine but rather a callable that returns a coroutine which doesn't return or yield anything. So the correct type hints for those would be the following I think:

from asyncpg import Connection
from asyncpg.pool import PoolConnectionProxy

setup: Callable[[PoolConnectionProxy], Coroutine[None, None, None]]
init: Callable[[Connection], Coroutine[None, None, None]]

Btw, we should probably add asyncpg-stubs as a dev dependency so that the type checkers will catch all this.

snow sapphire
woven tundra
#

I think Callable[[Connection], Awaitable[None]] would be preferable, as it's more generic

#

Doesn't really have to be a coroutine function; As long as it's a callable that returns something that can be awaited, it's fine

wicked abyss
wicked abyss
dusky lance
cloud helmBOT
woven tundra
wicked abyss
# 
Argument of type "(Connection[Record]) -> Awaitable[None]" cannot be assigned to parameter "init" of type "_InitCallback[Record] | None" in function "create_pool"
  Type "(Connection[Record]) -> Awaitable[None]" is incompatible with type "_InitCallback[Record] | None"
    Type "(Connection[Record]) -> Awaitable[None]" is incompatible with type "(__con: Connection[Record], /) -> Coroutine[Any, Any, None]"
      Function return type "Awaitable[None]" is incompatible with type "Coroutine[Any, Any, None]"
        "Awaitable[None]" is incompatible with "Coroutine[Any, Any, None]"
    "function" is incompatible with "None"
woven tundra
#

Should be Awaitable[Any] then.

#

Coroutine[None, None, None] should have also failed

wicked abyss
woven tundra
#

But that was Awaitable[None]

#

I'm saying pyright failing here is correct because Coroutine[Any, Any, None] is indeed not compatible with Awaitable[None]

#

But if Awaitable[None] fails, Coroutine[None, None, None] should also fail

wicked abyss
#

the first arg in the generics must match?

woven tundra
#

The first arg of Coroutine is the return type

#

Equivalent to the generic arg to Awaitable

#

Coroutine[T, Any, Any] matches Awaitable[T]

wicked abyss
#

what I understood, but phrased poorly

dusky lance
#

that one gets the directory traversal fix and 2.8.3 changelog into main

dusky lance
#

litestar#3478 litestar#3479 litestar#3480

cloud helmBOT
peak monolith
cloud helmBOT
wicked abyss
dusky lance
dusky lance
#

Are you guys happy if I merge https://github.com/litestar-org/litestar/pull/3484 without satisfying the code-coverage requirement? I don't want to add a pragma for no cover there, but I also don't want to have to write a test to hit that branch of coverage as a part of this PR. It is just that after the ruff update, we were able to remove a # noqa for a line that was otherwise already uncovered.

snow sapphire
signal linden
#

yep

dusky lance
#

thanks!

dusky lance
#

litestar#3486 - DTO data transfer when dto type is nested within a mapping annotation

cloud helmBOT
snow sapphire
cloud helmBOT
#
guacs
<:pull_open:882464248721182842> [litestar-org/litestar-asyncpg] fix: correct the type hints for setup and init

Pull Request Checklist

  • โ—ป๏ธ New code has 100% test coverage
  • โ—ป๏ธ (If applicable) The prose documentation has been updated to reflect the changes introduced by this PR
  • โ—ป๏ธ (If applicable) The reference documentation has been updated to reflect the changes introduced by this PR
  • โœ… Pre-Commit Checks were ran and passed
  • โ—ป๏ธ Tests were ran and passed

Description

  • correct the type hint for the setup and init in PoolConfig
Created ยท
snow sapphire
cloud helmBOT
#
cofin
<:pull_open:882464248721182842> [litestar-org/litestar] chore: unpin Advanced Alchemy

Description

This PR unpins Advanced Alchemy.

This allows version 0.9.3 to be installed with teh litestar[sqlalchemy] command group.

This enables the following:

  • A numebr of repository and service optimizations and fixes
  • SQLAlchemyQueryRepository from advanced alchemy
  • to_schema from the Service used there (see the fullstack repository for usage details)

However, this AA release also had to integrate it's own copies of the Litesatr OffsetPagination dataclass. Litestar will attempt to use this version of the dataclass when a supported version is installed. This is implemented similar to how the filters in filters.py work currently.

Closes

Labels

area/dependencies, pr/internal, size: small

Created ยท
snow sapphire
snow sapphire
cloud helmBOT
snow sapphire
#

I'll get a patch release out once this is merged

peak monolith
snow sapphire
snow sapphire
#

https://github.com/litestar-org/advanced-alchemy/pull/192

You can now pass default joins into the repo without having to use a custom statement. There's a load parameter that will accept a Load type (selectin,joined, etc..) , This will also accept the instrumented attribute you want to join. It will inspect the relationship and automatically choose between a joined or selectin load. Lastly, this will also accept a * to default to a joined load.

#

@mental quarry

#

@woven tundra @dusky lance this is one of the last "big" features I had on my list for 1.0

#

Here are a few examples from the tests for reference:

class Country(UUIDBase):
    name: Mapped[str]
    states: Mapped[List[State]] = relationship(back_populates="country", uselist=True)

class State(UUIDBase):
    name: Mapped[str]
    country_id: Mapped[UUID] = mapped_column(ForeignKey(Country.id))

    country: Mapped[Country] = relationship(uselist=False, back_populates="states", lazy="raise")

country_repo = CountryRepository(session=db_session, load=[selectinload(Country.states)])
usa_country = await country_repo.get_one(name="United States of America") # selectin load
assert len(usa_country.states) == 2 

country_repo = CountryRepository(session=db_session)
usa_country = await country_repo.get_one(name="United States of America", load=Country.states) #  automatic selectin load based on `uselist` in relationship
assert len(usa_country.states) == 2 

repo = USStateRepository(session=db_session, load=State.country)
string_california = await repo.get_one(name="California") #  automatic joined load based on `uselist` in relationship
assert string_california.id == california.id 

repo = USStateRepository(session=db_session, load="*") # joined load & unique() added to execute
star_california = await repo.get_one(name="California")
assert star_california.country.name == "United States of America"
mental quarry
dusky lance
cloud helmBOT
wicked abyss
dusky lance
#

absolutely needs tests ๐Ÿ˜ตโ€๐Ÿ’ซ

wicked abyss
#

AA test suite takes that long to setup :p

snow sapphire
#

15-20 min test runs for the full thing

dusky lance
#

i gotta sort that

#

still hoping for < 20 min though ๐Ÿ˜„

dusky lance
cloud helmBOT
dusky lance
umbral furnace
cloud helmBOT
#
jderrien
<:pull_open:882464248721182842> [litestar-org/litestar] fix(logging)!: do not log exceptions twice & fix `pretty_print_tty`

Description

Currently we're logging the exceptions twice: the full one and a truncated one.
This PR tries to improve this, which implies small breaking changes.

Almost finished but still a WIP, I need some feedback please!

Summary

โš ๏ธ The former code has been introduced to solve #1294, unfortunately I haven't been able to reproduce the issue. It's not clear in which cases it happened, or if the PR (#1296) fixed the original issue (I don't think so because exception are logged twice in the end).
โ“ TODO Anyway, to solve the original issue (#1294), truncating the request's body would have been a better solution. I'd be happy to dig a little bit more here. Any idea about how to reproduce the issue? (I already tried to write a test with a file upload, but the request's body doesn't get printed)
โš ๏ธ Type ExceptionLoggingHandler and signature of handle_exception_logging() get updated by this PR, seems better and more flexible this way.

Logging
โœ… Behavior is still the same, just slightly changed how we count the lines to display (see the code and the test test_traceback_truncate_default_logging).
โœ… Ability to set traceback_line_limit to None.
โš ๏ธ Default value for traceback_line_limit is None, I don't think we should truncate stacktraces by default.
โ“ Maybe we could deprecate traceback_line_limit to keep things in sync with Structlog. And if it's really something wanted by users, they can bring it back easily via a custom exception_logging_handler.
โ„น๏ธ Updated log messages to get something similar to structlog.

StructLog
โœ… Fixed a bug with pretty_print_tty (as a separated commit).
โœ… When pretty_print_tty=True, we don't log the stacktrace twice and the remaining one gets truncated automatically by structlog (see below).
โœ… When pretty_print_tty=False, exception gets logged as json. The stacktrace is not truncated anymore. It should not be a problem because the full stacktrace was also logged, so with this PR we're reducing the global log size. It's still possible to parse the JSON after, with some tools, to limit the lines displayed. If needed, the full stacktrace is still available.
โ“ TODO: If it's fine on your side, can I mark traceback_line_limit as deprecated and document it?

Extras
โš ๏ธ Note: A few tests use structlog.testing.capture_logs. When using it to capture the logging.exception() output, the traceback is not yet rendered. That's something to keep in mind.
โ“ What about turning log_exceptions to always by default? (Hiding errors by default seems a bad choice according to my sysadmin background).

Structlog: before & after

__With `pret...

Labels

Breaking ๐Ÿ”จ, Triage Required :hospital:, area/logging, area/types, pr/external, size: small, type/bug

Created ยท
umbral furnace
#

Oh, yeah, I wrote a long (but full) description... ๐Ÿ™ˆ

dusky lance
wicked abyss
#

I mean, enjoy freedomland

dusky lance
#

I did opt out on the evening's activities - wasn't feeling 100%

#

I sat at the same table as Guido at the typing summit today ๐Ÿคฏ

wicked abyss
#

now i dont feel bad they left you out

dusky lance
#

haha - the guys were off to an astral/ruff party tonight

#

did you see the update I wrote in #1240662108979597342 message

wicked abyss
#

yes, I am just looking at that

dusky lance
cloud helmBOT
dusky lance
cloud helmBOT
woven tundra
cloud helmBOT
#
provinzkraut
<:pull_open:882464248721182842> [litestar-org/litestar] feat!: Remove deprecated `litestar.middleware.exceptions` module and deprecated params of internal `ExceptionHandlerMiddleware`

Description

  • Remove deprecated module litestar.middleware.exceptions
  • Remove deprecated app param of ExceptionHandlerMiddleware
  • Remove deprecated exception_handlers param of ExceptionHandlerMiddleware

Closes

Labels

Breaking ๐Ÿ”จ, area/asgi, area/docs, area/private-api, pr/internal, size: small, type/feat

Created ยท
woven tundra
cloud helmBOT
woven tundra
#

@snow sapphire

snow sapphire
#

Looking good. If you hate the copywrite Linter error, I think we can remove it. I needed to start using it on something else, so I was testing it out there.

#

make lint should give you the same output as the 3.12 test

woven tundra
#

What's the reason for having it like that instead of single license file?

#

is there an advantage or something?

woven tundra
#

I don't think I understand how this projects build system is set up

#

Why are there 3 requirements.txt in addition to the dependencies in pyproject.toml? And how do I update them? And the makefile also doesn't seem to actually have a way to install the dependencies? But it did when I ran make install?

#

I'm lost ๐Ÿ˜ฆ

snow sapphire
snow sapphire
#

and I wanted to find a way to lint for it.

woven tundra
snow sapphire
#

Hmm, it's supposed to refresh the lock file using hatch-pip-compile

woven tundra
#

Ah, I see. hatch-pip-compile wasn't installed when I ran make install for some reason

snow sapphire
#

ah, you already had hatch installed

woven tundra
#

I had to do make install-hatch

woven tundra
#

๐Ÿ˜„

snow sapphire
#

๐Ÿ™‚

woven tundra
umbral furnace
snow sapphire
#

there's already a section for enabling features in experimental_features, so perhaps this is something that could be used there.

umbral furnace
#

Ho, I understand ๐Ÿ™‚ Thank you

snow sapphire
#

Re: this:

โ“ What about turning log_exceptions to always by default? (Hiding errors by default seems a bad choice according to my sysadmin background).

I think this is is a good idea. We have too many times had to recommend enabled DEBUG mode. I have it enabled in my config most of the time.

#

Great work on this PR detail! This is super helpful.

#

Am I correct in my understanding that this is the only breaking change as of the current state of the discussion?

Update of type ExceptionLoggingHandler and signature of handle_exception_logging() might break custom exception_logging_handler. This is the main breaking change. This might be reverted
#

ah, and the None

#

I think the None addition should only expand the type signature, so it is probably non-breaking re: the API?

umbral furnace
#

It depends what we call breaking change, tbh I'm not 100% sure ^^'

#

If strictly talking about the API, or other usages

snow sapphire
#

I'm in favor of getting PR merged; we may have to phase in some of the clean-up over time (exception_logging_handler signature & traceback removal) though.

umbral furnace
#

Current signature of exception_logging_handler can be kept for now, it won't affect other changes (I guess)

snow sapphire
#

Thanks for spending time on this one, btw. We need some polish to logging.

umbral furnace
#

Note I have another branch around logging improvements (as stated in the PR)

#

we might want to release as much as possible

#

(or not, I'm entirely sure)

snow sapphire
umbral furnace
#

I mean outside of the major breaking changes
Or for example, would you wait 3.0 for traceback removal? I'd think it should be released in the 2.0 branch (considering it's a duplicate of the exception, increasing log size, and so it might have a production impact)

#

Also, I've been told that LoggingConfig might disappear/be deprecated in v3... so adding breaking changes and at the same time deprecating seems weird to me

snow sapphire
snow sapphire
umbral furnace
snow sapphire
umbral furnace
umbral furnace
#

But I feel like the outcome of reducing log size worth it

#

(for a v2.x release, I mean)

snow sapphire
umbral furnace
snow sapphire
#

with the body logged in the response

umbral furnace
#

I tried that, but the request is not logged, at least with a default setup

#

is there an option for that?

snow sapphire
cloud helmBOT
#

src/app/config/app.py line 131

response_log_fields=["status_code"],
umbral furnace
#

I mean, if users log the response, they have to make sure to remove binary form data by themselves (?)

#

Oh, I see

snow sapphire
#

IIRC, there's a flag that only logs the response when it's above a certain size

#

and it only shows if you also log the body of the response

umbral furnace
#

Thank you for pointing this

snow sapphire
#

I think this is the biggest gripe I have with our logging. It's spread out too far

#

There's the config, middleware, request logging, etc...

#

I grew to like the LoggingConfig in a weirdly ironic way. It roughly mirrors the standard logging config components. (I'm not opposed to it going away though)

umbral furnace
#

Yep, that's not easy to understand when you start

#

Yep, it's not that bad because you can configure as you want, it just needs some small fixes
But having to support standard logging/picologging, structlog, and then people might want to add support for other logging libs

#

and then it becomes bigger & bigger

snow sapphire
cloud helmBOT
#

litestar/middleware/logging.py line 204

if key == "body" and response_body_compressed:
umbral furnace
#

Thank you very much!

snow sapphire
#

It's been a while since i looked at this, but I vaguely remember there being a size check to this as well. I'm not seeing it now though

#

these are the valid values you can log:
"status_code",
"cookies",
"headers",
"body",

#

you can also reproduce this error on the request side

#

if you upload a large file and enable body on the request

umbral furnace
#

I'll dig a little bit in the coming days (I'm in the CEST timezone, so going to disconnect soon)

#

But it was very helpful

snow sapphire
woven tundra
cloud helmBOT
woven tundra
#

Make our CI not fail on forks all the time ๐Ÿ™‚

woven tundra
cloud helmBOT
woven tundra
#

@dusky lance if you have the time to look at this ๐Ÿ™‚

woven tundra
cloud helmBOT
woven tundra
#

Some more routing refactoring

wicked abyss
#

for my understanding, was it a bad idea if it was Router.from_controller(controller) instead of controller.as_router()?

woven tundra
#

A bit. Router.from_controller requires the router to have knowledge about the controller's internals

#

Controller.as_router keeps all of that contained within the controller, and just uses the Router's public API

snow sapphire
woven tundra
cloud helmBOT
snow sapphire
cloud helmBOT
snow sapphire
cloud helmBOT
snow sapphire
#

@woven tundra re: this https://github.com/litestar-org/litestar/pull/3507

The addition of traceback is an unfortunate bug that I left in. I meant to remove this before since there's already a nicely formatted exception. The user who created this PR dropped into chat the other day as well.

cloud helmBOT
woven tundra
snow sapphire
#

It was definitely not intentional that I print 2 tracebacks with structlog. I noticed it was doing this, made a mental note to fix this, and totally bailed on fixing it.

#

I don't think this would be breaking because you have the same data printed twice. When the terminal is not in tty mode, they both serialize to json as well. However, the correct one has a bit more detail.

woven tundra
#

So we can safely remove the traceback?

#

Can you comment that on the PR as well, so it's documented? ๐Ÿ™‚

snow sapphire
#

I think so - for structlog at least.

#

yeah, i will

#

i'll link to our chat as well

umbral furnace
umbral furnace
snow sapphire
peak monolith
cloud helmBOT
umbral furnace
woven tundra
#

Seems to be all good @umbral furnace!

umbral furnace
#

Hi @woven tundra ๐Ÿ‘‹
Just seen your message
Thank you for taking a look, I'm adding a test for the coverage warning

woven tundra
#

You won't be able to get the coverage warning away

#

You have removed tests, which is intended

umbral furnace
#

(I'm also squashing commits and removing the ! from the commit message)

woven tundra
#

It's just an artifact of how that is measured

umbral furnace
#

I thought the cov warning is because I haven't tested a condition ๐Ÿค”

#

Well, let's see

woven tundra
#

Oh

#

Yeah, you're right! Disregard my previous messages ๐Ÿ™‚
I was looking at the wrong thing ๐Ÿ˜ฌ

umbral furnace
#

I did the final cleanup โœ…

woven tundra
#

merged ๐Ÿ™‚

umbral furnace
#

I'd like to open another (and last, at least for now) PR to improve LoggingConfig.
I've seen a few things to fix anyway, but I'd also like to add a new option logging_module: https://github.com/litestar-org/litestar/issues/3536
Would you accept a PR for this? It implies updating a few tests, so I'd prefer having an approval before opening a PR.

cloud helmBOT
#
jderrien
<:issue_open:882464248951877682> [litestar-org/litestar] Enhancement: add an option `logging_module` to `LoggingConfig`

Summary

It would permit to switch from picologging to logging without having to hack around or uninstall picologging.
It would be useful in tests but also in production (vs having to update a requirements file and rebuild an artifact).

This option would be optional. The default behavior would be the same as today (use picologging if the lib is installed).

Basic Example

std_logging_config = LoggingConfig(logging_module="logging")
picologging_logging_config = LoggingConfig(logging_module="picologging")

Drawbacks and Impact

No response

Unresolved questions

No response

[!NOTE]
While we are open for sponsoring on GitHub Sponsors and
OpenCollective, we also utilize Polar.sh to engage in pledge-based sponsorship.

Check out all issues funded or available for funding on our Polar.sh dashboard

  • If you would like to see an issue prioritized, make a pledge towards it!
  • We receive the pledge once the issue is completed & verified
  • This, along with engagement in the community, helps us know which features are a priority to our users.
Labels

Enhancement

Created ยท
snow sapphire
#

and I'm ok if I never see another lambda_stmt again.

woven tundra
#

Okay, I understand what this does, but can you explain why this fixes the issue?

snow sapphire
#

Yeah. But I can't explain why LambdaStatement behaves like this.

It's got to do with the way the lambda statement does caching of the actual lambda. Prior to this, we convert to a lambda statement right as we initialize the repository. This caused issues with caching when passing in options after the initial instantiation of the repo.

The fix was to store the initial statement as a Select instead of a lambda on init. Then we construct the lambda on each repo call before appending the filters/query changes to it.

#

The other issue was that we had statement caching disable in a few places because it seemed correct (count and list_and_count for instance)

#

however, when you do this, it messes up statement caching in subsequent calls

#

so, the last piece of the fix was to remove all lambda_stmt cache customizations by default.

#

This has the secondary benefit of fixing the issue that was opened 5 days ago that I never even saw

woven tundra
#

Oh wow

#

This sounds like a mess tbh ๐Ÿ˜ฌ

#

Maybe it's worth it to open an issue on the SQLA repo to get confirmation that this is actually working as intended? ๐Ÿ‘€

snow sapphire
#

yeah, I intended to. There are significant performance improvements when using the statement caching, but it's crazy complex

#

it makes it sometimes hard to tell if I'm using it incorrectly or if it's intended

mental quarry
snow sapphire
#

Yeah, I'm sure there'll be a replacement first though. The benefits appear to be quite significant

#

speaking of which, I think i ran across a fix for your open issue

#

I just need to go find it again

#

lol

#

for this

mental quarry
#

I was just thinking that this PR will likely fix that? Maybe not

snow sapphire
# 
@listens_for(Session, "do_orm_execute")
def _do_orm_execute(orm_execute_state):
    """Modifies queries globally based on rulesets."""

    # this rule ensures that records won't load if they're soft-deleted
    # unless execution options are set otherwise
    if (
        orm_execute_state.is_select
        and not orm_execute_state.is_column_load
        and not orm_execute_state.is_relationship_load
        and not orm_execute_state.execution_options.get("include_deleted", False)
    ):
        orm_execute_state.statement = orm_execute_state.statement.options(
            with_loader_criteria(SoftDeleteMixin, lambda cls: cls.is_deleted.is_(False), include_aliases=True)
        )
#

I'm not 100% sure if the latest release will fix this