🚀 Space Reviewers 👾 Episode 6

Speaker 1: Go ahead and get started. So hi everyone, we're space reviewers and I'm Lolian. And do you guys want to introduce yourself?

Speaker 2: Yeah, I'm Sage.

Speaker 3: I'm Raffaella.

Speaker 1: And yeah, the point of space review viewers is to show you that um When we do a peer review, you know, we all come from different levels of experiences, but we can all contribute something. So Yeah, it doesn't really matter um our level of experience. We all have something, some kind of insight, some kind of viewpoint. So, how is this going to work? We're going to pick a PR. We've already picked it out. And I'll share the link with you soon. And then we'll dive into it and walk through it And odds. Yeah, sure, our review. Um So we wanted to do this to show you that everyone could do a review and we'll show you our thought process and how we stepped through it.

Speaker 1: And also be respectful. Um we follow the Django Not Space Code of Conduct. Okay. So we have the link to the PR. Um to we have the link to the show notes. here um so you can follow that along and yeah and I'll go to the notes here So here are the notes. And Raffaella, would you like to give a summary of the ticket since you wrote the notes?

Speaker 3: Yes. Okay, this uh uh this secret is uh uh was open because uh uh when you have some um uh some settings inside the header of your uh of your proxy for example nginx is going to not to be um uh see the the port uh the the host uh if you have uh sorry if you have the host uh inside your header is not going to uh to evaluate the port, so it's going to be um uh not uh aware of the port. And this is uh the problem uh they have uh going to sol solve it. There's um at least

Speaker 3: one uh um one um one try with uh with the first PR that was made by the the sorry D G C G H uh and then uh there were lots of different uh approaches the um also the the last one was uh not the last one the previous um uh one was made by Apollo uh considering uh another uh another approach for the uh for the West for the for the other uh is that um uh

Speaker 3: completely enough uh

Speaker 1: Um yeah, that's good. Um Sage, do you have anything to add to that?

Speaker 2: Um yeah. Well, not not really. So this was new you previous PR and I think um There was no activity for some period of time, so someone else picked it up. And but yeah, I'm I'm just curious how We are going to review or test this, whether we are going to set up a Django project with an Nginx setup or Yeah, because it's it's quite I don't think it can be tested um like just with a plain Django uh run server. So yeah, uh I'm not sure what the plan is, but it'll be interesting to see.

Speaker 1: Okay. All right. Um I'm let's see, let's go ahead and pull down the code. So I already have it pulled down and so um so I'm on that PR right now. And So I think it's I guess let's look at the the most recent PR first and see what files were changed and then let's compare it to um the one that Apollo did and then we could kind of go from there. Um So for here, I think we could see like most of the files, they're the tests. And if we just like skim through it, we see um

Speaker 1: They're making small changes. This one looks like it's like making um adjustments to the test that just to make those tests work. And similar for here, I think these were changes, so it was adding in the allowed hosts. Um since I oh uh maybe I should actually start from this comment that was made. This this one is actually I'll I'll say this is a kind of a hard one to follow because there were a lot of PRs. But you know, you just kind of step through it one by one. And I think if

Speaker 1: okay, let me back up a bit more. So let's start with The first PR, I believe that that's this one. So um yeah, that's the first PR right here. So let's go here and then just skim do the comments. Um I wanted to go down to the one where they were talking about. Okay, so this is this is a comment that was talking about the logic for how these variables should be used in the precedents. So like Raffaella mentioned, um When you're using the use x forwarded host variable and when it's set to true, then you want to make sure that

Speaker 1: um use exported uh okay you want to make sure that when you have use exported host is true and use exported port is false then the settings in the Nginx server should include the server port. And then if you have the case where use exported host is true and use exported port is true. Then it doesn't need to use it doesn't need to uh let's see. Oh why is this

Speaker 2: Yeah, I I think um so in Ngin X you can set the header when you uh um set up the proxy from like the front facing server to Django and um You can retain the information of the host and port by setting these headers. And Django supports these headers by those settings, like use X for what it host and a ex forwarded port. So when you only set the X forwarded host to be true but the forwarded port is not used, then I guess uh this is the case where people

Speaker 2: just use that one header and combine both the host and the port in that header directly. But there are other cases where people Instead split the host and port into separate headers, so X forwarded host and X forwarded port So the expected engine X configuration when you have both settings in Django set to true is that you also set the headers separately. um the host use its own header and the port is also um with its own header. But yeah, there might be cases of misconfiguration where people set both Django settings to true, but they used the first

Speaker 2: um example of Ngin X configuration where they uh um concatenate the host and port into a single X formulate host um header in which case uh Um because if Django does not handle that, it might end up appending the port twice. Um Right. So we want so i in this case uh Marius is suggesting that Django should raise an exception if that's the case like if it encounters um a port uh in the header forwarded host um but the x forwarded port setting is set to I think that that's my understanding.

Speaker 2: I'm not sure if I if I get that right.

Speaker 1: That helps a lot. That makes it clear for me at least. Yeah, and then he gave links to the these variables in Django. So those are in the documentation and it explains how they should be used and the precedence in there as well. Um, and then there's more mm And then there is mention of like updating the documentation. to reflect the changes for these variables and the logic here.

Speaker 1: And let's see. I'll go ahead and jump back. That's I wanted to make sure that we captured that logic here. And then let's jump back to this PR here. So if we look, if we take a look at the changes here. So there's a change to The regular expression that's being used to parse the um the settings from the server. Um and that includes The port year, um and then And then

Speaker 1: this is where the bulk of the logic is parsing that server line. And here it's checking to make sure that that logic is um Is implemented as it was described

Speaker 3: Lillian, could I could I ask you something?

Speaker 1: Yeah.

Speaker 3: Uh on YouTube there Thibaut that asked uh if is there a cookie cutter quick way to set up engines and Django to try this out.

Speaker 1: Um I don't know. Do you know Sage?

Speaker 2: Sorry, just wanted to make sure. So that YouTube comment is asking a question whether there is a cookie color template. Or are they letting us know that there is a template that we can use?

Speaker 3: No, it it's uh it's a question.

Speaker 2: It's a question, okay. Um I'm not aware um if there is a such a template. I mean there probably is because it's quite a common config. But I guess We so yeah we could either try to look for a template that helps us test the changes, but I wonder if we could get away with just um crafting the request with the appropriate headers. And see what happens.

Speaker 1: Yeah, I was gonna go with just doing it in the tests too.

Speaker 2: Yeah.

Speaker 1: Um especially since they've written the test here.

Speaker 2: Yeah.

Speaker 1: Um so Should we go ahead and just run the test that they have?

Speaker 2: I guess we could start with that.

Speaker 1: All right, so I am on this PR and there are one, two, three, four files of tests. One thing I want to point out is that this request test is it looks like it's a brand new file, but it's actually not. There's a um a previous commit that removed the file because it was conflicting with the requests um request module, the request library, and so they renamed it to request test. So a lot of this should be in here. And Uh it makes it a little bit difficult

Speaker 1: to uh track what the changes are, but we'll see. All right, so I can run um I'll start with that one and then just to make sure that it at least passed. And then we'll go to request test and then that one passed as well and then I'll come back to the request Oops, site. Yes. That one passed. And then I saved the request folder last because it's not going to pass.

Speaker 1: Okay, so it's this is what I described earlier where it's conflicting. And so there was actually a change. So what I'm going to do is just rename this so that we can run it or to something different and then we'll run it and then we'll see So this actually failed, and the reason I believe is There's this test with a different encoding. So it uses encode Latin one. And I'm actually not sure if this is allowed

Speaker 1: according. Oh, actually it's not allowed. And it was actually fixed in the other tests. And I'll show you. Um if you do a little bit of digging on on track, you'll see um There was a commit. Um I'm gonna go back here take it. So this is raising an error. Charge it should be ignored for the application. And this is actually corresponding to this error right here. And

Speaker 1: so if we take a look here And the changes that were happening in there, they were happening in the request file. It made some changes into there And also into the tests and we were let's see. So we saw it was billing on Tests alternate post. I think that's a new oh okay so that's right here. That's that's the old test

Speaker 1: and then it was renamed And it was updated. Um yeah, so that's that's one thing where um this PR needs to be rebased. Um so I just wanted to clear that up and so that's one thing. So if I go into there and I go ahead and Remove this. I think we should be okay because we're not concerned about this. This is not related. It was already fixed in the other PR. And I'll run it again and then There are a couple of more problems. I'm just trying to work through all the problems so that we can get to the tests that we that are important.

Speaker 2: I think we could improve the fail output by uh so if you try to install a package called tblib, yes that one Um yeah, because sometimes when we run the tests in parallel and there are exceptions.

Speaker 1: Oops. Let me see. I was

Speaker 2: Oh, yeah, you see you fee.

Speaker 1: Okay, and then

Speaker 2: Okay, I can't.

Speaker 1: Okay, so that's the one that we saw earlier. And we know that it's been fixed in one of the commits, so we don't need to worry about it. And then this one. Um string string has string object has no attribute read. Um It looks like so it happens the the task exists in both of these And are

Speaker 2: so are are those two files meant to be merged and like it merged into a single one?

Speaker 1: Yeah.

Speaker 2: Okay.

Speaker 1: Um, I wasn't sure how to go about this other than like trying to comment out the ones working And so I'm gonna go ahead and comment that one up since we saw it was passing in the other one and then this one Okay, similar. This one looks like it's been rewritten. So I'll comment this one out as well. And then and then I guess it should all past now and so the tests that remain not all of them are new

Speaker 1: but I did do like a I just went in manually looked inside of them to pull out which ones were not um which ones were new and most of them started with this string here so test get port with exported host that one was new Um, so maybe we can take a look and make sure that it's testing the new implementation. Yeah, if if I'm going down the wrong path, just you know me to do something different.

Speaker 2: I I think that's sensible, uh, but I guess just to make sure we could take a look at the original a PR from Florian um Apollo 13 uh just to see the tests in requests the tests Um yeah, so so it seems there are some new lines in okay, so that's the test that uh was added, yes, and then Oh

Speaker 1: that's the same one.

Speaker 2: Yeah.

Speaker 1: There's that one.

Speaker 2: And then there's also some changes to test get port, I think.

Speaker 1: Yeah, most of so those are adding server names to them. I'm I'm going to separate it out. So there are tests that had minor changes and then tests with like that were new. Have it

Speaker 2: text.

Speaker 1: And then so I think this one was added. And I can get rid of it from here. And these were the ones with changes. And this one is just a change. Oh, it's the same one.

Speaker 2: I think there might be uh so do we also need to include the ones that just added the server name? I think uh there were a few uh

Speaker 3: get part with the ex forwarded part

Speaker 1: oh good one uh i missed that one

Speaker 2: Okay. Good.

Speaker 1: Okay, I think we got demo And

Speaker 2: all right.

Speaker 1: Um so should we start with one of these and then Run it and see or how should I go about this?

Speaker 2: Um yeah, I I think we could try, I mean We have already run the whole test module, right? So um oh but if you mean like going through the lines, yeah, I think like the test code instead of just running the test Right, okay, yeah, I think I think we can do that.

Speaker 1: All right. Um so this one test get port with exported host. Okay, it looks like it has the server name and the port in this variable. Um and so that should be supported and it seems like um it's checking If this is using the exported host header Okay, and so they've set up

Speaker 1: the metadata, the meta variable to have These other values and they're checking to make sure that this get port will return the correct port number.

Speaker 2: Yeah, so the the first part tests um so I think like the server name and server port that's like the standard uh meta request information and then if the request includes the X forwarded host header and then the setting is set to use that header, then Django should take that into account and use the word from the exforwarded host header instead, which in this case um that's the thing because um the the actual so server port that Django receives is 8080 but the header from XForded

Speaker 2: host uses 8000 And so we expect that if you use the export worded host setting, then that should be used instead. So that should be A But if a request comes in and it doesn't have the ex forwarded host, I suppose in this case the expected behavior is for Django too. use the default um surfer port information instead of I don't know maybe failing if it doesn't find the X forward host. I mean I I think this that's that sounds like a sensible test.

Speaker 1: Alright.

Speaker 2: What do you think?

Speaker 3: I have a I have a question. Inside the ticket I saw that the example port they they used is 8443 Uh yes over there. I don't uh let me know if that's correct. It's not a default port because the usual default port it's the 443 Is it correct? Um

Speaker 2: yeah, so

Speaker 3: it is

Speaker 2: uh like HTTPS port is usually four four three.

Speaker 3: Okay.

Speaker 2: And in this case it's uh so engine s engine X is listening on some custom port in this case as an example 8443 Because I think the issue was um Yeah, so the the issue is that So uh usually it's pretty normal to not include the port if if the scheme is uh HTTPS and the port is 443 because that's the default. default port or in the case of HTTP it's AT. So yeah, uh the the original ticket is talking about uh let's say you use um HTTPS and the port

Speaker 2: is some custom port not 443 in in this case 8443 and apparently Django just ignores it so um yeah that that's the issue sorry just does that answer your question

Speaker 3: Uh no because I uh I saw inside the engine's uh settings that um sometimes they suggest to use this type of port But it's uh not actually the default board for HTTPS. Is that correct?

Speaker 2: Um yeah, because HTTPS the default port for HTTPS is 443 without the eight at the front. So um I think with Nginx, um I'm I'm not entirely sure. Um Because you know you can technically just run the server on any port you like and I guess in this case um Wait, are are you saying that Nginx suggests using 8443 instead?

Speaker 3: Yes, I saw um I saw inside here yes I posted in the in the chat. Oh yeah, yeah, yeah. Over there. I

Speaker 2: Okay.

Speaker 3: Maybe is do you think it's going to happen to to have the age for three port as a default port? In the future maybe.

Speaker 2: Oh. Oh, yeah, I just realized. Yeah, that's that's a good point. I didn't re I didn't know that. Apparently , uh it's commonly used as an alternative port for HTTPS. Um but yeah I'm not sure how Th does web does the web browser automatically know um if you just type the address. Um I think I think it's probably just um because

Speaker 2: you know uh 443 is the default port and you might have some other application running. at that port. So yeah.

Speaker 3: So you have the true application on HTTPS. So you have the ah okay okay.

Speaker 2: You have to run them on different ports and um like a common alternative for to four four three is eight four four three. But I don't think uh if you type https sum server dot com for example I don't think the browser will attempt to connect to 8443. It will always try to connect to 443, I think. So it's it's just um some sort of I I'm not sure if you can call it convention. I guess it's just a common alternative for it. But yeah, I I honestly um I'm not a hundred percent sure on this. So yeah.

Speaker 3: Yeah, I I I found out just uh just in just when I um was watching the ticket. So I mm I would I became curious because I know that the default part was the um 443 but I was um I I don't understand if maybe in the future it could be also uh H um 443 and another default port. Who maybe I don't know. But but now yes, it it's it's the it's a custom port. Thank thank you, thank you, Sage.

Speaker 1: Good observation um All right, um let's see. So we were going to do that test. Um are we going down the right path, like going down each one of those tests?

Speaker 2: So I think the problem with the new PR is um It wasn't rebased in a way that was correct. Uh uh I I mean because we still see the requests tests. Um file in in in the requests directory when it should have been moved. So if we go to the new br um ideally we should only see the requests underscore tests directory instead of uh both of them. So yeah, I'm not sure Yes,

Speaker 2: I guess that's one of the main points that we can point out in the review comments. Like could you Um every base uh maybe every uh every every base well Yeah, it's uh it I g we we we just need to at least point out that this file has moved so we shouldn't have this file in the PR

Speaker 1: Okay, so that's one thing, and um Oh, and then another thing I want to point out is um they haven't added any documentation.

Speaker 2: Right.

Speaker 1: And so we'd like to see documentation. Documentation. Um Do we need to specify which files they should be updating?

Speaker 2: If we want to be extra helpful, I guess that that would be nice.

Speaker 1: It would be, I think it would just be these. Um this request response.

Speaker 2: Yeah, so I think that and maybe also the Settings documentation for the use X for what it port and yeah this one.

Speaker 1: Okay. Um do I need two point okay so that one docs Add documentations for settings. txt and um I forget request response Okay, yeah, dot TXT. And then

Speaker 1: Did we uh find out so I commented this out and Was that a test that they added? So okay, so for this one, I was wondering what

Speaker 3: No.

Speaker 1: When I ran it it failed on my end.

Speaker 2: I I think it's it might be because this is the old test before it was changed in the PR that this allowed this encoding for um form in URL encoded thing And the reason why we are seeing this as an added file is because it the rebase was done incorrectly. So we are seeing this as an addition. even though this test isn't related to the PR at all. So yeah, um Okay. Yeah. Oh

Speaker 1: I'll just add a note. Make sure to remove or or is the rebase just enough?

Speaker 2: It it depends how they do the rebase, I think, because um yeah they they have already rebased at the point where the PR uh um move the test so as you can see in the PR uh if we go to the new requests underscore tests the py file. So yeah, we can see that the the new tests have been added to to this file instead. So they should have just well actually no. Um so when they so as you can see the these only include changes where

Speaker 2: the changes were um Added well like it only changed the existing tests, but we don't see any new test cases in here. So the Um I guess um this is just me assuming what happened during the re the rebase. So I think git correctly handles uh like it f it can follow the rename of the test file and hence uh we we see these changes to the existing tests. But in cases where the original PR added some new lines uh for for the new tests, um Git will ask you like uh do you want to keep this file

Speaker 2: because in Django's main it's been removed. And then this person who made the new VR just accepted the whole test module instead of picking just the new tests and move them over to the new file. So that's what they should have done. Um but yeah, instead they uh just accepted the whole test file and this results in like the the old tests that are now in the new file is re-added in in this um uh old requests test. py file and us as reviewers can't easily see like

Speaker 2: which tests are new so yeah um I I'm not sure how to word that into like a review comment, but um yeah, so make sure that the requests tests. py file is removed but the added test should be incorporated to the um new file. Yeah. So I guess make sure uh to incorporate the new tests into uh uh the new

Speaker 2: yeah the new test into yes that file instead Um I guess we need to point out that uh yes, exactly Yeah, I think that works.

Speaker 1: Okay. Um Oh, I have another one uh documentation and version change and I just picked that up from one of the comments from um a previous one of the comments.

Speaker 2: Yeah

Speaker 1: Let's see

Speaker 2: So the does the original PR uh sorry, I I forgot uh I think you have a list of new tests that were added? Yeah, so I think we can just look at those and instead of like the whole thing because some of the other tests are just tests that have been there forever and they're unrelated unrelated to the VR.

Speaker 1: Okay.

Speaker 2: Yes.

Speaker 1: All right, so get host with use export and http host. Okay, so this is the case where you have the separate um the lines. Exported port header.

Speaker 1: We'll take a look. Um Um, if I'm understanding this correctly, it should well mm It should be using this value if it's present and then this value.

Speaker 2: I'm not sure if that comment is correct. in in the test like it says should not use the ex forwarded host but it does use it

Speaker 1: Um is it saying the HTTP exported variable?

Speaker 2: Oh.

Speaker 1: Versus the

Speaker 2: I guess. Yeah, I I think the comment should have said should not use the exported host header because the setting that's set to false is the host one but the port one is set to through so it should combine uh so it should use the host from http host but the port from x forwarded port I think so the test itself is correct, like the assertions are correct, but the comment is not a thing.

Speaker 1: Um should I see ya

Speaker 2: Oh yeah, yeah.

Speaker 1: Okay. Um

Speaker 2: Um I think we can just use the uh suggestion feature of that Yeah. So I think that should be host. Um

Speaker 1: variable?

Speaker 2: Um

Speaker 1: It's

Speaker 2: uh well I mean we could probably just change the um to keep the original comment but change the word port to host because um yeah we 're Yeah. So instead of port, yeah, it's it should say host, yeah.

Speaker 1: I'm kind of con

Speaker 2: Oh yeah. So that variable is like so Django transforms the header into the um http underscore thing in the request meta. It's up to you, I guess, if whether you want to work the comment in terms of Django request meta attributes or in terms of the header names

Speaker 1: Oh, okay, I see, I see. I'll leave it.

Speaker 2: Yeah, I just thought just to avoid making I mean because the the original PR already used the the uh terms in terms of the header names so just to uh Um reduced the amount of changes, I guess.

Speaker 1: Okay. So there's that one and then then I guess there's this one

Speaker 2: Um sorry, what's the test just before this? I just want to make sure uh just Okay. Do we need to be concerned about this? Um

Speaker 1: did

Speaker 2: anything change in this? I think yeah

Speaker 1: It was just the server names.

Speaker 2: Um, okay. All right.

Speaker 1: So this is the opposite of the other one?

Speaker 2: Yeah, I guess you can see that.

Speaker 3: Yeah, the note is not included now. Should use the forward port.

Speaker 1: I guess it looks good. Right. So we'll go to the next one. Um so this is with um IPv6. So they're using the IPv6 address. And they're just making sure that it can parse correctly. Um okay. Oh so they're checking that this will convert into a number.

Speaker 2: No, it's it's it's in the X forward. Yeah, that one.

Speaker 1: Oh, I see, I see. Okay. Um All right. I think that's similar to the other one. It's the IPM.

Speaker 2: Yeah, exactly. I think so.

Speaker 1: And then this one get port with exported host and port. So we're expecting it to use these two. All right, so these two settings are set to true here. And it's checking that um it's using that and this.

Speaker 3: Is this a case that um uh Felix was talking about

Speaker 2: um not yet. I think um That will be it might be the next one. Um Oh, uh that's the one after this. Yes. Yeah, that's

Speaker 1: Okay, so this one has the host with the port and then it defines the port again a second time.

Speaker 2: Yes.

Speaker 1: And I forget which one it should take.

Speaker 2: It should raise an error.

Speaker 1: Oh, okay.

Speaker 3: Okay.

Speaker 1: And so it raises the error.

Speaker 2: Yeah, I think uh we might be able to add a refew comments saying that I think usually Django uses assert racist message to also assert the error message. So not just the ever uh type

Speaker 1: should i go ahead and add that or just uh someone else wants to add it rafaella do you want to add that

Speaker 3: Um, so I I don't understand uh Mm

Speaker 2: they

Speaker 3: are going to Yeah, he's going to um um raise an arrow and

Speaker 2: Yes

Speaker 3: It's not what we expect. I think the yet

Speaker 2: uh it's just uh j Django usually um If it's an error message that is racist by us uh as in by Django and not like say Python. Um we tend to use assert races message instead of assert races so that we also ensure that the error message is a certain string that we added in the implementation. So I guess what we can do here is we check the implementation code where the error is raised. Yeah, so something like that.

Speaker 1: And I haven't looked into what that message would be.

Speaker 2: Yeah, we'll we'll we'll need to look into it

Speaker 1: I was just wondering like if someone else wants to add a comment.

Speaker 2: Yeah, sure. I I can add that. Yeah, maybe add that to the uh notes and then I'll add that at the end of the review.

Speaker 1: Um use cert raises message instead of So it raises in this test. All right. And so we gone to

Speaker 3: um Lilian if you want uh if because I'm I'm the in the same line I can uh suggest uh the or it or do you want to make a a message a long message or is maybe um I can use a suggestion

Speaker 1: you can you can make it

Speaker 1: There was one comment that I saw about this function here, parse host. It was asking to validate the value that was being parsed to make sure that it's a number. Did any of you see this one?

Speaker 2: I guess it makes sense. Uh did did um so I think Florian said that they would um push the Changes with those. Um okay

Speaker 1: I don't think he ended up pushing it because it never came in and I don't see it here. So I was just wondering if maybe we could resurface that and ask them

Speaker 2: about if

Speaker 1: they should validate the value that's being split.

Speaker 2: Yeah, I think it's worth a comment just asking whether it's something we need to consider. Just to make sure it doesn't get lost in between the transition.

Speaker 1: Okay. All right. What else should we check? Um should we go to how how do you feel about the review so far?

Speaker 2: I think it's going good. Um I probably look at the code from top to bottom, uh just in case. Um Okay, so just changing some imports. Um that still looks good to me so far. That I'm not sure about that. Why is there an a max allowed file? I don't think we're dealing with files, so I'm not sure why that's there.

Speaker 1: Interesting. Let's see if they had it in here. It wasn't in here and I wonder if it's in Maine. Um mean okay, let me just check if it's their requests. Okay , right. Okay, I have yeah, they don't have me neither. Is it being used anywhere?

Speaker 2: That's a good question.

Speaker 1: Okay, it's down here.

Speaker 2: I I don't know why that's there. Maybe they were working on an unrelated ticket. Let me see if I can find a ticket. A ticket about maximum files.

Speaker 3: Sage how how are you going to find out if uh a ticket has this um um this constant?

Speaker 2: It might not be that constant, so I'm just using Google to find like with keywords, for example, Django maximum files upload. And I make sure to only include results from code. jangoproject. com. I'm not sure if I'm able to find it though.

Speaker 2: Yeah, I cannot find that specifically There is a somewhat related ticket, but it's already fixed from five years ago, so probably not that. Nine years ago actually. But yeah, I don't think that's related to what we're doing at all. So yeah, I guess we could just ask. Is that related to this PR? Related to the TKID, I guess.

Speaker 1: That's a good catch. And okay, so another thing I noticed is that the previous one used lazy Regular compile. And then this one doesn't? Wasn't sure.

Speaker 2: I guess you could check what um the main branch is doing. And yeah, if it has lazy compile then we should keep that Yeah.

Speaker 1: Why was this removed?

Speaker 2: I I think I I mean I think the answer is it was lost during the rebate So

Speaker 1: oh okay. It it's interesting though because it wasn't

Speaker 2: oh that's good

Speaker 1: it wasn't here but It probably was lost at some point.

Speaker 2: Yeah. Uh what about the very first PR Okay, yeah, then I I think it's a good question to ask, like why is that removed? So maybe it wasn't a a big base issue

Speaker 1: Okay. Um let's see. And then they used They added a name tubble and I know Adam Johnson put in a comment about name tubbles. I wasn't sure. He says needy comes are weird. You suggested using something else?

Speaker 3: Oh, do you mean the first PR?

Speaker 1: Yeah, so this um um

Speaker 2: it's the second PR. Oh

Speaker 3: this is this

Speaker 1: And then also it's like from five years ago, so I don't know if it still applies.

Speaker 2: Um yeah, I'm not sure how it uh uh I guess um yeah we can just leave a comment um saying do we Uh should we change this to something else? Uh per comment from Adam? Should we consider, I guess, consider

Speaker 2: I think

Speaker 1: they got rid of this function.

Speaker 3: Oh mm. There's uh there's an idea to uh uh to m to keep uh uh get row host um but the Apollo said that um he preferred to have a function to have the same behavior in the previous PR was uh was a comment

Speaker 1: Um

Speaker 3: I think yeah, right there. Isn't this Yeah.

Speaker 1: Okay, so it's pretty much just rewriting this function.

Speaker 2: Mm-hmm

Speaker 1: Were there any test cases that we have not considered?

Speaker 2: That's a good question. I'm not sure. So

Speaker 1: if we look at this logic again , Didn't we look at the tests? Maybe we can get back to that after the rebate.

Speaker 2: Yeah, I think

Speaker 1: it'll be easier to follow. Is there anything you see in here?

Speaker 2: Um So I guess at first um Yeah, it it it checks whether so I guess the X forwarded host header is only considered if the use exported host setting is true. And and it does that and it has a variable that tracks whether the overdot host contains a port or not which I think is sensible. And yeah, if use export with a host setting is not set to true, it's just going to look in the HTTP host meta

Speaker 2: which sounds reasonable and otherwise um yeah if it cannot find the http host It does the logic ice which I assume might be in the original implementation uh in Rowhost maybe? Uh the the one that was deleted Yeah, so I guess that's the I wonder if um so if you click on the Cockwheel icon button at the top uh of the PR yes and then click hide white space

Speaker 2: I wonder if this gives us a better view of the oh yeah, okay. Yeah, I guess some of the lines are not changed Um okay. Yeah, so the first if um use forwarded toast and um Yeah, so that that bit is new and then the if HTTP host in well actually that's that's still the same uh as the deleted one, but instead of using settings.

Speaker 2: directly we use a variable for use for what the public host import. So yeah, I guess the change here is that we use the parse host header function instead of just reading the HTTP host, which makes sense. I guess that's where it tries to parse the port from the host. And that um Yeah, and then it adds the bit where it detects

Speaker 2: that you're using use X forward port set thing but um and then it also finds the um Wait. Yeah. So the use X forwarded port setting is true and then there's the X forwarded port header, but the X forwarded host also contains uh the port then that's improperly configured and that's the error message that we want to test Yeah, I think that makes sense.

Speaker 2: And I guess it caches the result. in the parsed host object attributes so that um yeah if if you already called the method before it It's just going to return the result instead of recalculating the whole thing again I see

Speaker 2: It it's mostly the same, um, but because of the added indentation. Um Or I guess um because we now parse the host thing um to ensure that the port is separated if it includes one. Yeah, now we make use of the parsed host object thing. But I think the rest of the logic is the same I think so

Speaker 1: Where is this validate coming from?

Speaker 2: Um just

Speaker 1: a nitpick up. I feel like it's

Speaker 2: maybe the the method now accepts uh Oh. I mean I think if we look at the method of this this new method, does it have

Speaker 1: Oh, okay. I must have missed it.

Speaker 2: Yeah, okay.

Speaker 1: Is um I don't know, maybe this is nitpicking, but um Validate is like a verb? Should it be more is valid or something or

Speaker 2: I think it's because where the uh The parameter dictates whether this method should also validate the host. By validate, I think it's going to check against the allowed host. um

Speaker 1: okay

Speaker 2: yeah uh so yeah it it will call the validate host function only if validate is true so that's why it's called validate So

Speaker 1: there's a new get host method and it uses that parse host header and then it returns the combined. So that was from the tuple.

Speaker 2: Right. I think the getHouse method is not new, but um yeah, because Git doesn't really understand what happens. Um Yeah, so there is at the bottom. So if you scroll down a bit, there it is. Um there removed lines, a few lines above this one. Yeah.

Speaker 1: Okay.

Speaker 2: So yeah. um mostly move to the get parse h host header thing because um because now get port also relies uh into checking whether the port exists in the in the in the host. So that's why they share um a base implementation Mm that might be a new method

Speaker 2: Yeah, that's the suspicious thing.

Speaker 1: And we we've touched on that split to main port, and so

Speaker 2: I'm not entirely sure why this has changed.

Speaker 1: Let's see what's in the

Speaker 2: oh. Yeah, let's see what's what it says in the original PR maybe. No comments

Speaker 2: Do

Speaker 1: we have anything to say about this one?

Speaker 2: Um I'm trying to think. Um Port. I guess I don't really have anything to say, uh, but maybe there yeah, I'm not sure I'm not exactly sure what changed here maybe there is additional handling if the port is not Um in the

Speaker 2: like it has the port equals port or empty string, which is not in the original implementation. So I guess that's That's probably although the original code also has so it does match. groups default Str empty string. So I guess uh the the different thing is it originally uses full match and now it just uses Re dot match. which I'm not sure of what the difference is but I guess that's the main one of the main changes.

Speaker 2: But yeah, I I don't know what to add there. I think we can leave it as is.

Speaker 1: Okay.

Speaker 2: Oh, that's nice. So yeah, um that debug thing because they moved the implementation of that to the HTTP request object its itself. Yeah. Um uh why I said it's nice is that I because I think there have been cases where I really need that implementation um where it includes the scheme and the host basically like the absolute URL and previously it's only available in this debug thing But they've moved it to the HTTP

Speaker 2: request. Yeah, it's just refactoring.

Speaker 1: And then these are the tests which we've gone through. Let's see. This is mainly adding in the headers. To make it work. Um I think making sure that it's valid.

Speaker 2: All right.

Speaker 1: This is uh hard to look at again.

Speaker 2: Yeah.

Speaker 1: Should we just uh go to the next one?

Speaker 2: I I think so.

Speaker 1: Okay, and then this is just adjusting it to make sure it will work.

Speaker 2: Yeah, I guess that was um because it now validates against the um allowed host.

Speaker 1: And is this trying to make sure it gets a new instance?

Speaker 2: Yeah, I think so.

Speaker 1: Otherwise it has like stale data carried over.

Speaker 2: Yeah, it has the cache uh parsed object thing.

Speaker 1: Okay, we're at the end. Um, so we have some comments. Um do you two want to go through them and add them to the PR?

Speaker 2: Yeah, shall we split them?

Speaker 1: Yeah. Um just call out him

Speaker 2: Um so Rafi, did you say that you were going to add the assert racist message thing?

Speaker 3: Okay, I think I already did.

Speaker 2: Okay. Let's see.

Speaker 3: Let's see. Oh

Speaker 2: Yeah, f feel free to pick ones that you want to comment and I'll I'll I'll take the whatever le w whatever is left.

Speaker 1: Okay. Yeah, Rafi, just call out which one you want to claim.

Speaker 3: Okay, I'm going to have uh the max allowed files because uh it was really interesting to understand uh uh why I I think it's not made on purpose.

Speaker 1: Okay.

Speaker 3: Or maybe or maybe if it's on purpose uh we don't understand why. It could be interesting to understand.

Speaker 1: I'll take the ones with the links so it's easy to put

Speaker 2: them in.

Speaker 1: And then we have this one, this one, and this one. So Sage , do you want to take those?

Speaker 2: Yeah, sure, sure.

Speaker 1: I can I can copy and paste this into our Discord.

Speaker 2: Oh that that would be nice. Yes, please.

Speaker 3: No, thank you.

Speaker 2: Thank you.

Speaker 1: Okay. And we're gonna count down, right?

Speaker 2: Yeah, but let me write the comment first, because I I haven't Okay, uh one second. Um

Speaker 2: Okay. Um I've done one comment. I have two more, I think. What's the cat? Okay. Then documentation

Speaker 2: Explored house What One second. Sorry, it's taking me a while to write the comments.

Speaker 2: Okay. Um maybe a bonus comment about Um the refactoring. Um it should probably be a separate commit. Um I know this is This change was from the previous PR, but before we merged this this Should probably be sub right commit. Okay, I'm ready.

Speaker 1: Okay, um I guess well let me see I need to scroll up. Um all right Oh, I forgot it. Um

Speaker 2: let

Speaker 1: me see. Um Okay, I think that's

Speaker 2: um maybe sh maybe we should say thanks for working on it. Yes.

Speaker 1: I give one

Speaker 2: eight.

Speaker 1: Yeah Um okay. Are you ready?

Speaker 2: Yep.

Speaker 3: Yes. Yes.

Speaker 1: One two? Three screen. Very cool.

Speaker 2: All right, nice.

Speaker 1: Um I think I I don't see yours yet. I don't see yours, Popola.

Speaker 2: Hmm. I can't see yours either.

Speaker 3: I s I can see yours.

Speaker 2: Have you submitted the review? Is it still like pending maybe?

Speaker 3: Oh yes, isn't pending? Uh which one is it pending Oh yeah. Why is it impending?

Speaker 2: So if it's impending you need to click the green button at the top right um in the files change yes that button

Speaker 3: Okay. Oh and here we are. I have to submit a view at your comment

Speaker 1: Okay, we see you now.

Speaker 3: Oh thank you

Speaker 1: Very cool. All right. Well, I think that's it. We'll wrap up and we did it. We reviewed a PR.

Speaker 2: Yay.

Speaker 1: Thank you everyone for watching. So glad that people came. Yeah, so hopefully we can do another one maybe sometime soon.

Speaker 2: Yeah, this was fun. Thanks for organizing this.

Speaker 1: Yeah, I mean it's so cool to be able to work with other people and learn instead of like trying to figure it out on my own.

Speaker 2: Yeah, definitely.

Speaker 1: All right. Oh sounds good