🚀 Space Reviewers 👾 Episode 1 - Django - Parameters are not adapted or quoted in Query.__str__

Speaker 1: Yeah. Uh thank you for joining us and also for your patience. Uh we are a fallen team and And we are delightful to uh uh to join us in our first uh record review um inside Django. And we have different for who doesn't know, we have different experience, but we uh start uh from to learning now uh about Django, Django Core. And we would like to to share our experience and what are we going to learn during our our path And our primary goal is to share

Speaker 1: also our experience and the fact that everyone can uh can review um a ticket and a PR in Django.

Speaker 2: Yes, and so how this session is gonna work, um right now actually I should probably I'm gonna unmute folks. Uh don't feel obligated to actually unmute. But alright, you can you can all unmute yourselves again. Didn't have the security settings properly tuned apparently. Um But lesson learned. So yeah, what the Rafael and I picked out a couple of PRs. We prioritized them. The one we settled on is ticket. Twenty-five seven oh five. Um and we have this issue here. Um and so We have this little one page document that explains the PR for everyone or the ticket, a bit of a bit

Speaker 2: a bit about the problem, and then So we're gonna suggest everyone reads this. We'll take some time to read this, to read the PR, read the code, um, and then we'll come back. And kind of just start our discussion. And Rafael and I are going to go through it and look at the code, look at the comments on it, see what we think Where our understanding is where the discussion on the the PR went, um and if there's anything that we feel like the community should consider or potentially change, we'll leave comments and if Everything looks amazing and Alex did a fantastic job. We'll let him know.

Speaker 1: How uh we choose the PR. Um we we try to to search for something, some topic, uh, some familiar topic, but uh uh we uh we will like to also um um evaluate some PR that is more uh um That we we we think uh is gonna be uh interesting for uh for explain for um c um for an online coin review. So there's lots of chance we um we would like to pick something that we could be uh interesting to sharing.

Speaker 2: Yes. Uh and so why are we choosing to do this online when we could just do the code reviews ourselves? The thought was that it might help others see what the process is like and try to make it a little bit more accessible, especially considering Rafael and I are not experts with Django or the Django's code base. that this might prove to others, hey, you can like if you're looking for something to do, this is a possibility.

Speaker 1: But we are just today we are just going to uh choose the the previous the PR. It's just chosen. But If you want to suggest uh uh a Pierre, please please suggest everything you can uh think is suitable for this uh for uh for for our uh for our curta view. And um we take that in consideration. So we m Uh please take your time to to suggest something if you think it's gonna be suitable.

Speaker 2: And oh that um so on the left here, none of this stuff's really super easy to retype, but if you go to the Jenga Not Space slash space reviewers page. There at the bottom there's a link to submit that form. Yeah, I'm going to try to keep this panel on the left up constantly for people if you need notes or to catch back up or But yeah, lastly, we are operating under the DjangoNot Space Code of Conduct. There's a link to it here. If you go to the DjangoNot Space program channel or repository, you can find a link to the code of conduct. All right. Cool. So with that, um so the issue we're looking at is ticket 25705, which is from

Speaker 2: Dimitri? That's I think this is pretty old. One of the reasons I wanted to do this is that I might have suggested to Alex that this should be an easy thing to change. And then he spent several months working on it and caused him a bunch of strife and Carlton a bunch of strife. And so I felt like we needed to pay him back a little bit. Like Join in that. And so primarily the as I understood it, when you print out a query from a query set, it tries to print out the SQL, but that SQL can't be copy and pasted into a database shell immediately. So for example, if you have this query where you're trying to find users based on the date joined. That query when it gets printed out

Speaker 2: just has the date as text. And with the database, you would actually need to wrap that in quotes to actually make it turn into a value. Um this is called, I believe it's called string interpolation. This issue actually like the the original reporter wanted to be able to do this of take a model. Get the query get the string of the query and then plug that back into a raw call and then be able to get a query set back. Um this is not where the PR turned out to be. Like the the direction I'd end up going is to basically be um being able to view the queries that are being executed and being able to copy and paste those into

Speaker 2: uh a shell. Uh sorry, a database shell. And so like for me specifically, like I would use this to then plug it into explain analyze somewhere else. um and not have to do any adjusting on stuff. Um so but the discussion on the issue, which was pretty long. Um just quick scroll through it. It kind of varies into a couple different topics. So what we tried to do is Yeah. Summarize it down into where the PR ended up and then follow that thread all the way back to the beginning And so there was a different discussion that we identified here in the other topics area of the SQL with paramps. Like another ticket got opened up because of this ticket, but we're ignoring all of that discussion.

Speaker 2: So Alex identified that we can't only Postgres SQL and MySQL have a magrify function. Which allows you to generate a SQL query effectively, but Oracle and SQLite don't, so you'd have to do that manually. And that's probably a security concern. And then also Alex then determined like the magrify function wouldn't be great for this. So that wasn't even an option in the first place And so they ended up landing on the idea of adding a last query or an executed query property to the query set. There Simon agreed to it, Alex was on board with it, and Alex created a draft PR. That went through a couple of reviews, and now it's to the point where it is

Speaker 2: Pretty close to production ready, and so we're gonna take a look at it. Yeah, so if everyone have let me get this PR, the chat. Alex says that he did not work on it that many months, but he was just procrastinating working on it. Maybe, but I still feel like the pain part was true. Um yeah, so I I think right now let's take 10 minutes to everyone can just kind of read through the PR, look at the code. Um Try to get an understanding of like what is it doing might also be worth the look at the ticket. Not super important for everyone else to grok it. This is mainly for Raphael and I.

Speaker 2: One of us may not have looked at the PR yet and is coming into this fresh. So I I need a little time to get familiar with it. I wanted a little notes thing here. So I can

Speaker 2: Keep track of my thoughts. Do

Speaker 1: you usually prefer the to write uh on on the paper or uh or l take some notes on the uh on notepad or something or other apps.

Speaker 2: Yeah. I I don't know. I like text editor. It's just it's quick, easy.

Speaker 1: I think I'm gonna be the only one.

Speaker 2: How are you doing, Rocella?

Speaker 1: I think respect uh of uh what Alex said before, I think there's a lot of work uh uh on this PR because I think there's a lot of uh a different uh suggestion made before this uh PR, di a different kind of um approach that have been made.

Speaker 2: Yeah, agreed. I was looking through it and looking at the tests, the last one. Um It proves that if you have a prefetch which does a second query, like so um if you evaluate a query set that has a prefetch related, it does two queries, and it'll only return last query is only going to return the main model that you're selecting, not the prefetched one, or not the prefetched query. And I would wonder do we want to include the prefetches as well? Because if we Yeah, I it

Speaker 2: It makes sense why it's last query, but at the same time, if you're running a query set and using the ORM I think there's a reasonable understanding, like you would want to know what all happened there. Um Does that make sense?

Speaker 1: So you think that the query set need to be evaluated first?

Speaker 2: I'm saying I think I would wonder how people would feel about changing last query here to last queries. And then somehow If you would access it here, so there's um uh query set with prefetch, so it has a prefetch on the related model, and then when you evaluate it It's doing two queries. If you access the executed query for that query set, because it actually ran two queries to the data, if there was two queries that hit the database Should you not be aware of both of those? Like is it actually helpful to hide the second one?

Speaker 2: Or however many there actually are, because you know, you can have as many pre-fetches as you want.

Speaker 1: Do you think it's uh it's better to have um two uh two query uh so uh we can change the um the last query and uh um uses last queries to have um two queries

Speaker 2: Yeah, it would be a variable number of queries. But I think that it's gonna be a challenge It would be a challenge to figure that out because The way we're how right now we're just looking at to get the last query, it's grabbing the last one. There's no way to know how many queries were generated from which query set. We're just kind of assuming that there's only gonna be like they're gonna happen in order. Well, I think maybe I could be wrong on how queries works. It's a query, so this is actually let's go ahead and

Speaker 2: Hold this code. I don't think I actually did that yet. Um all right, so Alex said I can see logging and a query of query said. Um I can see the logging of every query set, every query the query set make makes being a possibility. Some notes you'd have are if the query set is evaluated multiple times, does each time get added to the list?

Speaker 2: It's a good question. Um the implementation gets complicated since in the case of five prefetches, you'd have to pick the last five executed queries. Yeah, and that last thing that you mentioned there, Alex, of How do you know which queries are associated with which evaluation of the query set? And yeah. I just want to explore this a little bit. I I I don't want to spend a ton of time on it, but all right, so that's not so it's We reslog this gets added to.

Speaker 2: Yeah. I don't think we're gonna figure I'm not gonna be able to figure that out of how this would work. Um I think it's worth us mentioning to see what others think. But if we maybe we just assume we're gonna stick with the last query API interface. Um because that is an improvement. Like it's Yeah. And then you can still grab the other queries if you need to. Like you can go to the connections and dot queries and that has all your everything that's been run. Uh anything else that you spotted that you want to talk about? Or what what did you all spot that you want to ask about?

Speaker 2: Can

Speaker 1: can I go first?

Speaker 2: Mm-hmm. Yes, please.

Speaker 1: Okay. I uh have some um I would like to understand better the um um the process because I think uh uh there's multiple um steps um to um um To give a query set. Okay, and um which kind of step uh is the query evaluated

Speaker 2: It's a good question. Okay. So this part of the docs It changes to light mode. There we go. So there are a couple of methods specifically that force evaluation. So there are like git or latest. or earliest. Those ones are going to force it. It's going to go fetch a single row and pull it out. So it'll immediately evaluate the query set. Or it'll turn the query set into a query, run that query, return the result.

Speaker 2: with uh collections of data, it's when you start iterating when you evaluate a query set like it's As it says in the the docs here, where you have a for loop going over it, that's when it first makes that first iteration, it's going to make it's going to generate the query, so it's going to turn to a query. The query will be run against the database. We'll start pulling data out. Another way is if you pass it into a list like here, that'll automatically evaluate the query set. There's another yeah this one where you convert a query set to a Boolean that I think that can also evaluate it because it's gonna call dot exists on it.

Speaker 2: uh which will basically say is there it'll do a query where it says limit one and if there's a piece of data that if a row does get returned it's like all right there's something some data for this query so if you ever have a a method Let me see. You have a method that does like user dot objects. all um or sorry it should actually be Git user query set. And you want to know, like let's say git user query set here. has the option uh has the possibility of returning none and you want to check that like you actually have to check if query set is none

Speaker 2: You can't do a a truthy check where like if query set because this is going to convert the query set to queryset. exists. Does that make sense? Um

Speaker 1: Yeah.

Speaker 2: Yeah. So it Yeah, so that's what it means by evaluating query sets. I kinda forgot why where we were getting onto that topic.

Speaker 1: I think that was my support and my question.

Speaker 2: Okay. Um Yeah, so I took a couple of notes. What other notes do you have before I dive into mine?

Speaker 1: And I uh I put a note because uh uh I saw that uh uh the PR relies on uh um on a connection that queries that is uh um available only uh in debug uh debug true

Speaker 2: Yeah. And that was um I think they mentioned that Simon mentioned this in one of his comments as well Of where was that link? I think

Speaker 1: that 's a good one.

Speaker 2: This is the one I I was so the debug cursor for queries logged. So it's not only just a bug, it's if this forced debug cursor gets set, which I don't I've never seen this. So it's Uh see if I can find it in the code. So okay, so this must be Something somebody else yeah so it's not showing up anywhere in the code except for the test um okay so this is a test

Speaker 2: thing Interesting. Yeah. So I think that's what you said too of Raphael. He pointed out that The documentation would need to be updated about um this only being debug. I think it would also work with I I feel like the Django debug toolbar has a debug cursor that sets I would imagine that sets it to true. Um I could imagine somebody eventually extending

Speaker 2: the the shell to allow for that like to to be set like through a setting or um through a command argument and then it kind of just goes and sets the database cursor. But I yeah I don't know how realistic that is. Yeah, so we we need to note that The debug constraints should be documented. Cool. What else?

Speaker 2: Sarah says with the prefetch thought would execute a query still make sense or base query deprecate it for a list? Yeah. We'll get back to I don't I think we're gonna ignore that for now of what the the API should be and just kind of assume. We're gonna pose that question, but we're gonna assume that we're gonna stick with last query for now. Anything else, Rafael?

Speaker 1: Uh no.

Speaker 2: Okay. Um so the ones that I was looking for so Alex in his dis original description had called out Some SQL compiler usages and I remember seeing those from some other stuff that I was doing and they were it was always really complicated. Um You mentioned looking for backwards incompatibilities, so I wanted to look for that and then There's also a comment somewhere about async not working. And so I want to check that. And then I don't remember where I saw it, but Al I think it might have actually been in the ticket. Excuse me. Alex had mentioned Last query, or I guess in this case it's now executed query. When that isn't set, if the query set hasn't been evaluated, should that return none, or should that raise an exception, or should it return an empty string

Speaker 2: Um and I just wanted to see where we landed on that and um yeah, I think those and then the other note I have is the prefetch thing, which we already talked about. So Yeah, looking through the code, there wasn't anything about SQL compiler. I think that must have been a mention of something before an earlier solution that he then found a better one. Backwards and compati incompatibilities, I don't I don't see any Because it's just adding on to the existing functionality. So I think we're okay there. Uh

Speaker 2: async, I would like to ex I I feel like it should this should work with async. I'd be surprised if it didn't. Um and I'd want to know why it wouldn't. And then yeah, with last query, I think this is I think this is worth a question to clarify why did we land on none rather than the exception or string. Do you have any thoughts on on on where is it? This piece of code right here. Where it goes else none

Speaker 1: Do we think it's more um um uh the the decision made uh on uh on something that you expected or maybe it's just an an arbitrary decision

Speaker 2: I don't think this was an arbitrary decision. I just don't I couldn't find the So Simon had originally suggested it. And okay, oh, so here it is. So this this is Alex. That's for copy and pasting. Um Okay, so Alex did consider raising an error if the query set hasn't been evaluated instead of none, but he's not a a big fan of that idea. Okay.

Speaker 2: As a developer, Rafael, what what would you expect to happen?

Speaker 1: Yeah, if I have this last query, um I expect to receive the query, but uh if none, um Maybe because there's no query or uh there's no chance to um um to retrieve the query

Speaker 2: That's a good yeah, that's a good and interesting insight. Like so there is a bit of a difference there of the query hasn't been executed yet, or is something set up where you can't get the query? Um So I I think if we look at queries log So how does this all right so there is a maximum length to it? Where does this come from? Oh, okay. So this isn't all right, so it's 9,000. There's no chance of it. You have to Okay. So I was gonna say like if this is something that the user can configure, how many queries you can log. It's possible that they get the

Speaker 2: get it into a state of misconfiguration. And in that case, maybe we do want to warn the user or log something out and say like, hey. You're not gonna be able you're never gonna be able to look at a query. Um, but that's not the case. So yeah, I think none is probably fine. Like there's Does a query set ever evaluate? Like does a query set ever boil down into a query that's none where you could Get the wrong impression? I don't think so. Sage asks, what happens if you do mymodel. objects. none? That is a really good question.

Speaker 2: I remember my Keep doing this, I'm gonna have to actually put together a sample project. Yes, maybe that's what we'll do.

Speaker 2: That's interesting. Okay, so that gave me an error trying to print the query. From bangs. Oh. How do I get the the connection dot queries?

Speaker 1: Okay.

Speaker 2: Seems like it is. Um Okay. Yeah. So you dot objects dot none does not generate a query. What if all dot none Okay, interesting. So I I've definitely had this pattern before where I I have a query set that's getting built

Speaker 2: Um maybe like on a view, like there's the default query set that you have as a member, and then there's some authorization logic that happens later. And I determine like, hey, this user shouldn't have access to this. So I'll adjust the query set to be query set equals query set. none to effectively restrict their ability to view any data. So is that when that gets returned, are we considering that to be an evaluated query set?

Speaker 1: Uh one question Jim, but this uh this case uh that you mentioned before isn't going to happen in the bug false?

Speaker 2: Uh, no, because the no, so this dot qu hm I don't think so. I could be wrong. I thought the connection here dot queries would be pulling from the same spot. as here. Um, but I guess I don't know for sure.

Speaker 1: Okay, Alex says that in the bug false the property will always be none because these queries are not logged

Speaker 2: Okay. Yeah, I I guess I wasn't sure if this queries log, which is what This is built on top of um if that's what's being exposed in connection. queries But I I would have to imagine it is. It doesn't make it'd be pretty weird if we had two different collections. Yeah. Hmm. I guess if we're not logging a query for dot none

Speaker 2: If it doesn't show up in connection uh in in dot queries, what Oof. Yeah, this is a bit of a problem because so let's say If you have um I'm trying to think of how to explain this. So if we have multiple queries being occurring in the same thread Is this going to pull whatever the last one is? Um is this based on the query set?

Speaker 2: So uh how do we check this? I suppose we can run the test. So I want to know if we do

Speaker 2: Arguably this should be done. Oh , still have to fix that. But I suspect if we if we run this test, I think it's gonna want to evaluate um to this. So let's execute a query after the query set is evaluated, but not when it's accessed. Correct. But Alex, I it's not, is it resetting um Is it going to reset the the queries collection?

Speaker 2: Yeah, as Sage said it might give us the I'm wondering, yeah, Sage is pointing out it is it gonna give us the previous non-empty query instead? So now I have to I have no idea how to run the test. Um Chango

Speaker 2: Uh sir, in test we have a certain number of queries. This is debug helper in the shell. Sarah, are you wondering if we should have this debug helper in the show? Okay. Hm. Well, I mean there's Yeah, I guess I would just wonder like what's the difference between that and running it with Print SQL enabled.

Speaker 2: Tests, queries. Okay.

Speaker 1: Sage suggests uh queries uh queries dot test dot executed query test.

Speaker 2: Thank you, Sage. Okay. Yes. So it does look like it pulled the previous one. Um I always forgot to change this again.

Speaker 1: Do you expect the the test failed?

Speaker 2: Uh unfortunately, yes. I didn't want it to fail. Um I it it shouldn't fail, like because theoretically if you call dot none or you call something that evaluates it It's gonna have to reset executed query. Like this this should this should pass in my opinion. But instead it's gonna pull from this one So it's it's using the previous evaluated query. So I think that's um a bug that we need to note.

Speaker 2: Um okay. Okay. So we have that We never did get to the async stuff. Yeah, so I mmm how much longer do you want to go, Rafaela?

Speaker 1: How many notes do you have?

Speaker 2: So we have two specific things we have to note. We have The debug constraint should be documented, which I think Raphael has already pointed out. And then we have this bug with objects. none of should what should this How does using the dot none query set method affect executed query? And then pointing out there could be use or utility in executed query being executed queries and showing us the full slate of queries that were executed, including the prefetches. So I think those are the the main notes that I've identified in our conversation.

Speaker 2: I would recommend everyone else who's had comments in the chat, please go leave a message uh on the PR. Um, I think Rafael and I are going to limit ours to those three that we talked about. But it's definitely folks can leave other comments. I'm sure Sarah Boyce doesn't need us to tell her that. She'll just, yeah. Okay. So Rafael, last time we did this, we both split the comments. Do you have one that you would prefer to leave more than the other?

Speaker 1: I uh um the the first one related with the documentation think it's already done. Okay. So we have just two Two no okay. And

Speaker 2: oh so it's the objects dot none and then Including the prefetch queries.

Speaker 1: I think I'm going to the the none.

Speaker 2: Okay. All right. And I will work on writing up a comment about prefetch prefetch queries. And like this is one of those things where I don't know for suggest like does the prefetch queries belong in the track ticket or does it belong in the PR? Because now it's This is kind of like what we ran into a little bit with the our our our test run of we ended up saying like hey I don't know if we agree with this API as a whole Yeah, we'll see. I guess we'll learn as we keep going.

Speaker 1: And because the none um is related with the test, uh do you think it's um uh the It's the it's something that it's could make different uh to to make the this this note

Speaker 2: Yeah, I think we're Yeah, I don't know where that's gonna go. Um I can share that code though if you want to include that. Um Uh but yeah this this area of the code that I'm sharing right now of self. executed equals connections db last query. This is probably We're gonna have to reset self dot executed query somewhere. Um

Speaker 2: And my guess is it's gonna be dot none. Sorry, I wanna see if I can Okay, maybe it's here. So this is the the none method? So we can either do it so set empty And what are we doing? So connections last okay. So I don't know if that would work. Um so you one option is just easily do This

Speaker 2: Or or or so this chain method every time we call it, that's actually probably where it should happen. Because every time you If you adjust the query set, you shouldn't be copying along the executed query, because you've now changed the query set in some fashion and it needs to be evaluated again Let's yeah. So maybe this is where it it belongs. Yeah, and like this exactly this this makes a lot of sense of return a copy of the query set that's ready for another operation. At that point, yeah, we we

Speaker 2: after that next operation is applied, that's when we should be clearing executed query. But I don't know enough of If this is actually right or not. But I think the idea makes sense. Anything else on that

Speaker 1: So I'm going to uh uh to leave a message with uh uh the uh the test we uh we tried uh

Speaker 2: Yeah. And if you want to mention that we should I w I I imagine we also need to test As you chain operations to a query set, is that last x is the executed query being cleared? Or is it always Yeah, and see like that's the confusing execute a query does imply that every time you change it, it should be none because you need to execute the new query set Um to get the the correct the now executed query. Oh, I'm so sorry Alex. Can't believe I

Speaker 2: suggested this and said it was an easy change. It is not.

Speaker 2: So I I don't know if anyone else does this, but the the Hemingway app can tell you how complex your writing is. And so Sometimes I like to plug it in there and see if I'm gonna be understood or not

Speaker 1: I remember you suggesting it this um the the the Hemingway but at first I I I just wondering why Wait the the cause I think it's a writer. It was a writer.

Speaker 2: Yeah, th there's another tool for writing, yeah. But that one that one's I don't use it for grammar, it's just more of like Tim, you're being way too wordy. Um okay. Uh any concerns, Rafaela, with me posting this comment?

Speaker 1: I think I have the uh a really simple comment if you want. I uh just post it on the chat.

Speaker 2: Which trap?

Speaker 1: Disc do you want Discord?

Speaker 2: Uh I mean it might be worth to put it in Zoom.

Speaker 1: Okay.

Speaker 2: I don't know how many people are monitoring everything. Insert the test button. Yeah, I think it would be I think you should also note that That this test highlights that the executed query isn't being reset. Um, and then pose the question of Should it be reset for every chained operation

Speaker 1: I also uh brought the executed queries could be reset to being chained on the next operation. Is that correct?

Speaker 2: I think so. I mean we never confirmed it, but I think it's a fair assumption. Because it's it's setting executed query when the fetch happens. Yeah, yeah. No, I think you're correct. Start defining a new query set And you never actually execute the query, it should return none, but it's going to return the previous value. Yeah. So you're good.

Speaker 1: Are we going to post it together?

Speaker 2: Yeah. Let's do what was the what do we say? We we screamed when we hit comment? Isn't that what we did last time?

Speaker 1: Oh I don't know if uh everyone is um agree with the with our suggestion. If everyone agrees.

Speaker 2: It's it's us. They're they're the peanut gallery. Or that's not actually no, that's not a right term to use anymore. I apologize. Um they're they're an audience. They they don't get final say. All right. Three, two, one.

Speaker 1: Three.

Speaker 2: Two. One. All right. I'm sure that that's exactly what everyone wants to hear. Um cool. I think that's it. If folks have any comments and you're not a troll, we would love to hear them. Yeah. Thank you everyone for joining. Thank you, Rafael. I had a lot of fun.

Speaker 1: Thank you, Sage, because you agree with us.

Speaker 2: Yes, yes. Thank you, Sage.

Speaker 1: Thank you, everyone.

Speaker 2: And thank you, Alex, for putting together this PR. It's really great to see uh movement on this issue. All right. Everyone have a good weekend. Bye-bye.