June 14

Monday TODO list:

• Work on DbValueV1 serde renaming.
• Work on changing Result<bool, _> to Result<(), _> in kanidm_client::asynchronous.

Monday morning started with me pulling changes from the master repository onto the master branch of my fork. I opened a branch for dbvalue-serde-rename so I could start doing renames, but of the first issues I encountered was that I didn’t actually know the shortened field names actually stood for. Names like CR, RU, and SP were not particularly self-explanatory. I was mostly able to overcome this issue by using VSCode tools to find all code referencing those variants, and then infering the real names based on context. Some names had very little context, so I ended up leaving questions in the comments in hopes of getting some feedback in a PR.

The PR was super easy to set up! My code passed the test and clippy, so I just sent it off to be reviewed.

To tackle my second daily goal, I setup another branch to resolve the bool-to-() issue (#472). I started out by doing a quick search-and-replace for the return values, and changing all the map(|_| true) calls with map(|_| ()). This passed the type checker with flying colors! Sadly, a lot of the tests failed, because some JSON thing was getting decoded and it expected a unit but found a bool. I got very stuck searching for the cause of this. I ended up creating a draft PR asking for help on how to get past this error.

June 15

Tuesday TODO list:

• Implement feedback from Monday’s PRs

I started off the day by implemention William’s feedback on the serde renaming issue, #477. It was mostly touching up a few things, so the changes went relatively quickly. One thing I was confused about was that GitHub said one of the tests failed, so I ran it on my machine and found that a different test failed. However, I ran them again and all the tests passed. I wasn’t sure how to pinpoint the issue, so I just left a comment on the PR in hopes that it was insignificant, and later figured out that it was a faulty test case.

After the new commits, I switched gears to work on the bool-to-() issue, #479. William didn’t seem fazed by the failed tests cases and even seemed okay with merging the changes, which I was very confused about. He did leave suggestions for two new PR’s, though: removing the OperationResponse type, and refactoring data streams (like thing().iter().map(some_fn).filter(other_fn)).

One of the things that really bothered me was the over use of closures when they’re not needed. For example, take the following from kanidm_client/src/asynchronous.rs:

let opid = headers
	.get(KOPID)
	.and_then(|hv| hv.to_str().ok().map(|s| s.to_string()))
	.unwrap_or_else(|| "missing_kopid".to_string());

There are two unnecessary closures here, which we can remove:

let opid = headers
	.get(KOPID)
	.and_then(|hv| hv.to_str().ok())
	.unwrap_or("missing_kopid")
	.to_string();

I realized this sort of thing showed up a lot more than I thought, so I decided to open a new branch and go on a tour around the code base cleaning up all these issues.

Another pattern that I found a lot was the following:

let auth = match wa.do_authentication(client.get_origin(), pkr) {
	Ok(a) => a,
	Err(e) => {
		error!("Failed to interact with webauthn device. -- {:?}", e);
		std::process::exit(1);
	}
};

Which should really be

let auth = wa.do_authentication(client.get_origin(), pkr).unwrap_or_else(|e| {
	error!("Failed to interact with webauthn device. -- {:?}", e);
	std::process::exit(1);
});

However, this only really works when the Err case doesn’t have return, break, or continue, since you can’t change an outer function’s state within an inner function idiomatically.

I ended up spending nearly 3 hours combing through files, and I think I only made it about halfway through the code base. I wasn’t really sure if this was a good use of my time, but I know it would improve code hygiene and probably needed to be done by someone eventually.

June 16

Wednesday TODO list:

• Continue the great refactor
• Implement feedback on PR’s

I started off my morning by continuing work on the great refactor. I realized after about 45 minutes that this task, although very satisfying, wasn’t really a great use of my time, since it probably getting all compiled to the same assembly. I decided to just create a PR (#482) for what I’d already done on kanidm_tools in hopes that it would get merged.

After that, I had some issues with pulling the merged changes from #477 from the master repo onto my fork. Fortunately, I was able to fix it after some googling around.

I was still having issues fixing #479 from Monday where a JSON object is failing to get decoded, so I sent William my issue. He was able to figure out the issue and give me some clues, which I was able to use to solve the issue in only a couple of minutes! I was able to figure out that the issue was a server-side error since all my changes passed the type checker, but he pointed me to the file and location where the server was sending a message. From there, I was able to dig through some function calls, where I eventually found some map(|()| true) calls that I just removed, and this fixed everything!

Later that day, I also was happy to see that my kanidm_tools PR got merged to master. Awesome!

June 17

Thursday TODO list:

• Submit issue for more fragile tests in kanidm_client::asynchronous.
• Code review for Oauth2 support #485
• Resolve OperationResponse removal issue #484

The incentive for creating more fragile tests in kanidm_client::asynchronous was because the cause of the error I was having with JSON objects on Wednesday seemed to appear in many functions in kanidmd/src/lib/core/https.rs, even though I only had to change one function to make all the test cases pass. This is bad, because it meant that some functions could be changed to have incorrect behavior and not show up in testing.

I had already written up most of the issue for more fragile tests, so I went through https.rs to create a list of all the functions that I changed, and submitted the issue on #486. Easy!

I started the code review for #485, and while I was able to give feedback on the Rust code specifically, I felt like I didn’t really know what I was actually looking at. So I took a little break to watch some YouTube introductory videos to Oauth2 to get a better idea of it, with the intent of being able to ask deeper questions and provide better feedback. I was able to pick up on the big ideas and basic terminology, which I think helped me give better feedback, but I still wasn’t quite comfortable with the topic.

The code review ended up taking me three hours before I called it quits for the day. I had a ton of fun reviewing, though, and I think I’m starting to figure out how to give better feedback!

June 18

Friday TODO list:

• Finish code review for Oauth2.
• Begin deprecation of OperationResponse.
• Write up and submit blog.

I took the morning off, but started my afternoon by finishing up my code review for #485. It took me around an hour to finish. I’m still blown away by the sheer amount of code that William was able to produce, even with prior research and experience! I ended up asking a lot of clarifying questions, and I think I was able to provide some little nitpicks for areas to clean up too.

One thing I’m really curious about is taking advantage of Rust’s borrow checker more throughout the code base. I noticed that there are hardly any references used anywhere, and that almost everything gets cloned around if it’s not directly moved. I think doing more research on replacing cloning code with borrows could be a big contribution, since it would bring performance gains to Kanidm, and because it’s very Rust-knowledge heavy, which I see as one of my strengths. Definitely something to look into next week.

After code review, I turned my attention towards my next TODO. I opened a new branch and just deleted everywhere that said OperationResponse, and all the tests still passed. Did this mean we’re okay? Who knows. I opened a PR (#489) and sent it off. The whole process took me less than an hour, demonstrating how unimpactful the type was I guess.

After I sent off the PR, I decided to wrap up there and write up this blog post for the week.

Weekly Reflections

Reflecting on the goals I set myself at the beginning of the summer, the Oauth2 PR was a huge step in my progression towards learning how to ask good questions and give good feedback. I found it really intimidating to review so much code I knew so little about, but I found that I was able to learn a lot along the way and ask good questions to help my understanding.

I was also doing so much with GitHub this week! I opened multiple issues and sent many PR’s (even though not that significant), and I was able to navigate my way around git with moderate proficiency as well!

Finally, I’m exciting to start thinking about how we can use Rust’s more advanced features (refs/lifetimes) to potentially speed up Kanidm. I think this will be a huge step in increasing my proficiency with the language, as well as making significant contributions towards the project as a whole. Exciting stuff!