swarm/api: bug fix exact match for manifest#15329
Conversation
nonsense
left a comment
nonsense
left a comment
There was a problem hiding this comment.
Overall looks good to me. I have added only one minor comment with regard to tests.
There was a problem hiding this comment.
I think it would be good to test the else-condition:
else if !multiple && entry.Status == http.StatusMultipleChoices
in order to cover incorrect response status if we didn't expect multiple choices status, but got one.
There was a problem hiding this comment.
Added! Thanks for comment
| if entry.ContentType == ManifestType { | ||
| err := self.loadSubTrie(entry, quitC) | ||
| if err == nil && entry.subtrie != nil { | ||
| subentries := entry.subtrie.entries |
There was a problem hiding this comment.
loadSubTrie() returns no error, but probably it should? The underlying errors that would bubble up would be from readManifest() Have it been evaluated whether they are relevant for this case?
There was a problem hiding this comment.
?
func (self *manifestTrie) loadSubTrie(entry *manifestTrieEntry, quitC chan bool) (err error) {
looks like it does return an error to me?
There was a problem hiding this comment.
Sorry, I meant the error is always nil the way it is now
There was a problem hiding this comment.
Still not sure I understand:
361 func (self *manifestTrie) loadSubTrie(entry *manifestTrieEntry, quitC chan bool) (err error) {
362 if entry.subtrie == nil {
363 hash := common.Hex2Bytes(entry.Hash)
364 entry.subtrie, err = loadManifest(self.dpa, hash, quitC)
365 entry.Hash = "" // might not match, should be recalculated
366 }
367 return
368 }
if loadManifest returns an error, it would be assigned to err on line 364?
There was a problem hiding this comment.
Jeez, forget I said anything. I'm a moron. You are right, of course.
| func TestExactMatch(t *testing.T) { | ||
| quitC := make(chan bool) | ||
| mf := manifest("shouldBeExactMatch.css", "shouldBeExactMatch.css.map") | ||
| trie, err := readManifest(mf, nil, nil, quitC) |
There was a problem hiding this comment.
Because this is exactly what caused the error we are trying to fix: if there was a file "shouldBeExactMatch.css" and a file "shouldBeExactMatch.css.map" (.map files are often generated by SASS generators for CSS and are actually used in development only, but that's how we found out: our own webpage would not load...), then if the client requested "shouldBeExactMatch.css", the result would be 300 Multiple Choices, instead of serving the file itself
|
@fjl this is approved from Swarm team. Feel free to merge when you think is appropriate. |
4 participants
This PR fixes a bug where a request for
/styles.csswould fail with status code 300 if the request resolves to a manifest and the manifest contains the exact match (empty path in sub-manifest) and more entries, for examplestyles.css.map