feat: Support for wildcards

[angrymouse]

No description provided.

[angrymouse]

As per #26, however without "overall" improvement of just allowing to put id into index, limiting to 256kb or so, only wildcard resolving part

[arielmelendez]

Thanks for the PR! I'm kicking off some discussion internally around whether to accept this PR pending a few decisions, e.g.

• should we accept a PR that only partially implements the spec updates outlined here: feat(middleware): path manifest fallback to * #26 (comment)
• should a partial or full implementation of said spec updates require stricter manifest version checking
• are there tests that need to be added/created

[angrymouse]

Let me put my 2 pennies.
Regarding 1st, I don't think it has to be in 1 PR, due to how different points in this summary are - for example, 256kb limit has nothing to do with wildcard support except that it may optimize it.
Also I don't think it requires new manifest version either, as it doesn't overwrite what's already done and isn't breaking change. Replacing "path" to "id" would be breaking change, but not adding wildcard.
Tests are up to you

[djwhitt]

@angrymouse Really appreciate you digging in on this! My initial thoughts:

• A version bump is important because it communicates to those parsing the manifest what they can expect to find in it. It's difficult to scale manifest processing if gateways have to parse the entire manifest to make that determination. Gateways can also potentially use the version number to communicate to their users what features they support.
• The 256kb limit isn't something that needs to be implemented as part of this. That's an expectation I think should be included in the associate spec updates though since it's hard to make promises about how quickly or whether manifests of unbounded size will be processed.
• We do not want to support paths for wildcards. Supporting paths is a major performance issue for on-demand parsing and ideally shouldn't have been part of the manifest spec in the first place. We could use the wildcard change as an opportunity to drop support for it for indexes too and make manifest processing simpler for gateways in the future. I think that's still up for discussion though. The main thing is that I do not think we should make the problem worse by supporting paths for wildcards.

tl;dr - great work so far, but let's switch to IDs for wildcards and include a version bump + check as part of this so that users can communicate their expectations about manifest features. We can continue discussion on whether we want to drop paths for index.

We'll also wants tests too, but we can dig into that after we're through discussing the functionality.

Oh, PRs should be against develop as described in the contributing doc (https://github.com/ar-io/ar-io-node/blob/main/CONTRIBUTING.md#pull-request-checklist), but that's just for future reference. This area isn't under heavy development, so we can easily rebase and merge into develop when it's ready.

[angrymouse]

About 3rd, I don't think there will be a lot of issues specifically for wildcards in supporting paths (because wildcard is processed as the last thing, after every path was checked) @djwhitt

[angrymouse]

Bumped version to 0.1.1, made resolving with only id possible

[djwhitt]

About 3rd, I don't think there will be a lot of issues specifically for wildcards in supporting paths (because wildcard is processed as the last thing, after every path was checked) @djwhitt

This is only true as long as we don't enforce path sorting. If in a future version we do as the original comment suggested and only support on-demand parsing with sorted paths, we can stop parsing early.

Also just to set expectations - there will probably be a little pause on this over the weekend, but we'll definitely pick back up next week and keep it moving.

[djwhitt]

@angrymouse Apologies for the slow follow up this week. We were fighting some ops battles yesterday. I think we can move forward with this after a couple adjustments:

Bumped version to 0.1.1

Let's bump to 0.2 since we're adding new functionality as opposed to fixing issues (we might as well follow semver unless we have a specific reason to do something different).

Also, now that we have consensus on the changes, go ahead and add tests to cover the wildcard functionality.

Finally, just so everyone following along is aware - once the dev work here is completed, there will probably still be a bit of a delay before this is available on arweave.net. For clarity we may also want to update the ANS spec before rolling it out there.

[djwhitt]

Hey, @angrymouse. Anything else you need clarity on from us to wrap this up? I think all we need is the version tweak and tests and we can get it into develop.

[angrymouse]

Hey @djwhitt ! Sorry didn't check out... Will do rn

[angrymouse]

@djwhitt done

[angrymouse]

Hey @djwhitt , any updates?

[djwhitt]

@angrymouse Thanks for the tweaks! Just one more request on the tests - can you include a test for the case where the wildcard uses a path instead of an id since that's still supported in the code. We'll be good to merge after that I think.

[djwhitt]

@angrymouse I just pulled this to see if I could add the requested test myself and discovered the test you added is failing. I'm happy to merge when that's debugged and the requested test is added.

[angrymouse]

Thanks, I'm not good in writing tests

[djwhitt]

To be clear, I was requesting that you debug and fix the test before we move on. I'm not actually certain the code is working in it's current state. Did you manually test it somehow?

[angrymouse]

Hey @djwhitt , no, I didn't test it and I don't recall you asking for that. Also, to make it clear, it's just an open, enthusiastic, non-inventivized effort to help your software, meaning I don't really have to do the corporate logic like tests

[angrymouse]

But the error seems to come from types, not the logic

[angrymouse]

So I'd prefer if this discussion went without "requested" and some accusations of me not doing enough QA on my code before PR - I don't work for AR.IO and don't get anything off this PR.

[djwhitt]

I'm not trying to make accusations. It was just unclear to me if the code works, so I was curious how or if you verified that yourself.

PRs are work for the maintainers. If we get a PR and we're uncertain that it's working it's going to take more effort to integrate it. We will still eventually integrate it assuming it's in line with the the project goals (this one is). It's just that the more work there is for us to do on the PR, the more it becomes subject to our (ar.io) priorities rather than the person making the PR.

But the error seems to come from types, not the logic

That's just because we run build before tests in the actions. If you run the tests locally you'll see a different error. The first error you'll see is from malformed JSON in the test. If you fix that you'll see another error which I haven't dug into yet.

[angrymouse]

Will look into fixing it, thanks for understanding

[djwhitt]

@angrymouse Thanks! I know PR processes can be frustrating too. I appreciate your patience as we continue to refine ours. 🙏

[djwhitt]

@jim-toth Happy to have you dig in on testing this if you have time (since you called it out as blocking). Though, be sure to coordinate with @angrymouse to avoid duplicate effort.

[djwhitt]

Closing. Release 14 includes support for fallback paths (see https://docs.ar.io/manifests/ for details).