Make the data prefix optional in profile imports

[MythicManiac]

Make it possible to import base64 encoded profiles which don't have the data prefix as the data prefix does not provide much practical value but it does make debugging profile issues more difficult as decoding them can't be done as easily with 3rd party tooling.

This is a forward looking change and does not impact anything in the immediate future. Once this change has been rolled out, it's possible to remove the prefix from the profile generation logic.

[anttimaki]

• This would match any valid one character substring
• This would allow padding characters anywhere on the string
• This would allow unlimited amount of padding

[MythicManiac]

This is intentional, we don't have a reasonable way to validate the contents other than attempting to parse them, so doing more than this would be over-engineering IMO. The main thing the validation filters out atm is e.g. empty response bodies and other related network level errors. If the response looks like it even might be base64, it's fine to pass it along IMO

[anttimaki]

Granted, this will catch empty strings, but e.g. "404 Not Found" or "a ½!#¤%&" will pass. Having a function named isValidBase64 that does absolutely nothing to check the validity is misleading. Based on what you say I would rather yeet the function and check against an empty string and then try to parse it. Or change the regex to /^[A-Za-z0-9+/]+={0,2}$/ so the function can at least say the data is not empty looks like it could pass the parsing.

[MythicManiac]

This is bikeshedding as is already, my point is that if the request resolves to invalid content that's fine, it'll fail in parsing and that is also fine. I'd much rather remove the entire check instead of making it less readable, but it might be useful as it is which is why I left it in. It certainly won't do any damage, whereas a filter that's accidentally too strict would.

I can think about the validation logic more if you want me to spend time on it but I didn't consider it important since as noted, it'll fail in parsing if it fails and that's fine.

[anttimaki]

• Taking into account your comments, I still think that objectively this is a poor quality PR. This is inefficient and confusing way for checking that the response is not empty
• You would've probably made the suggested changes faster and with less wasted energy than you spent on defending the current implementation, and I don't quite know why that is?
• I think my comments have been valid and reasonable and I don't appreciate them being called bikeshedding
• Saying "I can think about the validation logic more if you want me to spend time on it" comes across as playing a martyr to me.

[MythicManiac]

All of your comments are valid and not incompatible with my point that it doesn't matter if the validation gives false positives. False negatives are harmful, which is why I chose to keep it lax. This is the premise to why I objected making the validation stricter: it provides no benefit but it can be harmful.

The reason I called this bikeshedding is because we're talking about making validation more strict around something that doesn't even need validation in the first place. In other words, the thread is focusing on something we shouldn't be focusing on. In my vocabulary bikeshedding translates to misfocus rather than being unreasonable, but perhaps I should use more explicit wording in the future. If we had moved on to asking why does it have validation in the first place if it apparently isn't needed I wouldn't have considered it bikeshedding.

With I can think about the validation logic more ... I meant to offer an alternative to removing the validation altogether in case you'd prefer keeping validation around anyway, though now that we've talked about it and I looked into the bigger picture a bit more IMO the only improvement that can be made to it is to remove it entirely.

As far as this PR goes, my take is that either we merge it as is or we update it to remove the validation entirely. But at least under my perspective the current state of the code is only improving the previous state of the code so making it a blocker goes against my review philosophy. That said, I'd probably still want to update this to remove the validation given that there's no urgency with this either so there's no reason to skimp on quality.

[anttimaki]

I am fine with removing the validation, and tried to say as much in my second comment. That will solve the issue of having misleading code that might be seen as doing something it actually doesn't.

My understanding of bikeshedding is wasting time discussing an unimportant topic to avoid discussing the important topic because it's too difficult. And if the important topic for you here was not making the validation unnecessarily strict that's fine, but I don't think it renders my comments an "unimportant topic", but a separate one. After all, if catching confusing/misleading code is not within the purposes of code reviews, then I don't know what is.

[ebkr]

To add to this, if we were to validate it'd probably be better on the host endpoint side as opposed to in the manager - the export upload could just return a non-200 code instead.

The #r2modman tag was only added as a way to identify r2mm uploads to the old host vs manually adding a file as it was just a pastebin clone

[anttimaki]

It's your funeral.

[ebkr]

Nit but this would be more consistent were it defined as a function instead. I don't think there's any real difference between the two other than hoisting but it at least makes it clear that it's a function rather than a variable