Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add handling of arrays in form/query data like 'arr[]' #2247

Merged
merged 3 commits into from Feb 17, 2019

Conversation

Projects
None yet
2 participants
@schveiguy
Copy link
Contributor

commented Jan 2, 2019

Fixes #1984

A few notes about this:

  1. I didn't see any unittests to modify, all the unittests in here just check for compilation issues.
  2. This should work with arrays nested in structs, but not arrays of structs (unless the struct is completely serialized in one go). This is intentional, as the appending depends completely on serializing the whole thing at once.
  3. DictionaryList requires a @safe delegate for getAll, but there's a lot of unsafe things within the webConvTo function, so I couldn't do it. TBH, I'm not sure its really the right approach to use the =void, and use that very unsafe copy mechanism, but I'm assuming there's a good reason.

Please double check the test I wrote for when things can't be serialized. I think I got it right, but I didn't test.

@schveiguy

This comment has been minimized.

Copy link
Contributor Author

commented Jan 2, 2019

BTW, bikeshedding of dAppend attribute welcome, I don't like it either.

@schveiguy

This comment has been minimized.

Copy link
Contributor Author

commented Jan 4, 2019

The travis build failure seems unrelated, like a network failure.

@schveiguy

This comment has been minimized.

Copy link
Contributor Author

commented Jan 10, 2019

I'm going to start depending on this in my local project since I need it. Please let me know if there are any objections or changes that need to be made here.

@schveiguy

This comment has been minimized.

Copy link
Contributor Author

commented Jan 21, 2019

ping @s-ludwig

@s-ludwig

This comment has been minimized.

Copy link
Member

commented Jan 23, 2019

Just an idea that I haven't fully thought through, but I actually wanted to implement this kind of array specification a while ago. My plan was to just detect the style used and keep supporting both ways under the same NestedNameStyle.d name. Do you see any issues with this? I would say that since we are just talking about the receiving end that it should be fine.

@schveiguy

This comment has been minimized.

Copy link
Contributor Author

commented Jan 24, 2019

So there are decisions to be made with the API the way it currently works and adding in arbitrary appending to the array.

  1. The usual mechanism used to find the elements is first to build the variable name (e.g. arr[0]) and then search the query parameters for that variable name. This doesn't lend itself well for blended names (i.e. having both arr[0] and arr[] in the query).
  2. The dictionary list gives no way to get multiple identically named items out of it without using the delegate mechanism, or without allocating a separate throw-away array EDIT: there is, see comment below, so you have to make a decision early on whether you want to search for the non-indexed items (you can't just say "give me the 1st instance of this name").
  3. Should the underscore mechanism also receive treatment this way? You could just include the name with no _0 after it, or maybe just with a trailing underscore, and it could be the same thing. Seems like it would be inconsistent not to do this.

What I could do is change this PR to first search for the array using arr[], and if I find that, use the delegate range mechanism, and ignore any indexed ones. Another possibility is to search for both, and if I find both, throw an error. Though it may be useful to have blended items (i.e. you have arr[0], arr[1] specified anywhere in the query, but then the others are all arr[] and are just tacked onto the end). In that case, I can search for indexed items first, and then search for non-indexed ones. There may be a weird inconsistency if you had for instance arr[0], arr[], arr[], arr[2]. In that case (as it is today), the arr[2] would be ignored, only the first 3 would get in, and the array would actually have a 3rd element, but it wouldn't match the arr[2] parameter.

All of this also falls apart if you have an array of structs, as you can't use the non-indexed mechanism to specify an array of structs, I just want to make sure you agree with this.

I'm willing to do all of this work, just let me know what you think is the best way, and will get merged :)

Note, a way to get a range of elements that have the same name from DictionaryList would be super-useful, and would avoid the burden of requiring the calling code to be @safe. I'm willing to do that as well if you will accept it. There could also be a way to get the nth indexed item of a given name out of the list. So for instance get("arr[]", 0) could do that (though this is lesser performing since every lookup is O(n)).

Any other ideas, I'm game. I really need this functionality.

@schveiguy

This comment has been minimized.

Copy link
Contributor Author

commented Jan 24, 2019

Actually, looking at the DictionaryList, I can simply do byKeyValue and filter on the key name. So a range possibility already exists. I can switch to using that instead of the delegate.

@s-ludwig

This comment has been minimized.

Copy link
Member

commented Jan 26, 2019

I'd generally say that the mixed case is somthing that is rather "nice to have", but not at all required (ignoring one of the two, or erroring out would be okay if there are gains in terms of performance or implementation complexity). Supporting the append array style for the underscore convention would also be "nice", but mainly if the changed implementation makes that trivial.

Speaking of the implementation, to me it looks like the whole thing should really be turned upside down - iterating over the whole array using byKeyValue once and inserting into dst on-the-fly. There'd just have to be an additional flag per field of some sort to error out on missing fields. But I wouldn't mind keeping the current implementation and just changing the array part either.

Anyway, following that byKeyValue route, the most obvious implementation for handling the mixed case appears to be to let arr[] append and arr[i]=value would mean if (i >= arr.length) arr.length = i + 1; arr[i] = value;. The semantics that would result out if this seem relatively intuitive to me.

@schveiguy

This comment has been minimized.

Copy link
Contributor Author

commented Jan 27, 2019

Speaking of the implementation, to me it looks like the whole thing should really be turned upside down - iterating over the whole array using byKeyValue once and inserting into dst on-the-fly. There'd just have to be an additional flag per field of some sort to error out on missing fields. But I wouldn't mind keeping the current implementation and just changing the array part either.

Yeah, I think I will try that approach and see how it goes. It's bound to perform MUCH better, no more linear searches for each item, and I can probably avoid building strings to search with as well.

@schveiguy

This comment has been minimized.

Copy link
Contributor Author

commented Jan 31, 2019

ping @s-ludwig I have updated the implementation based on the discussion. This was a REALLY hard thing to get right. Please don't merge yet, I haven't tested it. I have to think about how to unittest this. I just wanted your review.

I didn't update the struct-handling branch, it still searches the same way it did before for members.

How it works now:

arr[] will now append to the array in question if all slots have been filled. Otherwise, it fills a slot that hasn't been specified. So for instance: arr[0]=hi&arr[2]=there&arr[]=vibe turns into ["hi", "vibe", "there"]

Unspecified slots are left as default values. So arr[0]=hi&arr[2]=there will result in ["hi", null, "there"].

I also specified a max array length for indexes as 2^16-1. This is to prevent an attack of something like arr[40000000000]=crash.

If an array element is specified twice, the second instance is ignored to preserve existing behavior.

If an array element beyond a static array length is specified, it's ignored to preserve existing behavior.

@schveiguy

This comment has been minimized.

Copy link
Contributor Author

commented Jan 31, 2019

Ah, one thing I just realized I forgot -- the static array branch should return error if not all elements are filled in...

But that's a minor change, will work on it maybe tomorrow.

@schveiguy

This comment has been minimized.

Copy link
Contributor Author

commented Jan 31, 2019

Will work on the compilation failures...

@s-ludwig s-ludwig referenced this pull request Feb 10, 2019

Merged

Auto-detect OpenSSL 1.1 #2190

@s-ludwig

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

I would probably have handled the mixed cases just the way that yields the shortest code, but if mixing the two styles is considered an actual use case, the new semantics are probably more useful, so I think that looks fine, too.

Do you think that you'll be able to tackle fixing the compile error (+ add some tests) in the next few days? I'm planning to tag a new release and it would of course be nice to get this in (since the next stable one is probably a bit further off).

@schveiguy

This comment has been minimized.

Copy link
Contributor Author

commented Feb 11, 2019

I will work on this and try to get it done in the next couple days. Sorry, I've been busy with other work.

I would probably have handled the mixed cases just the way that yields the shortest code, but if mixing the two styles is considered an actual use case, the new semantics are probably more useful

I can't tell what style is going to be used from the enum (I'm assuming you mean both append and indexed style), so I have to look for both. Plus you had said that it would be desirable to have sparse arrays, so I implemented that too.

I suppose the easiest thing to do would be to just try the arr[] name first, and if that works, use the range, otherwise use the original code. What I like about this implementation though, is we aren't building tons of intermediate throwaway strings.

Note, the part I wrestled with the most is that when you have a simple type (one that is parsed from a single string), then you don't really want to recurse and search the array again. So I thought it would be good to split out the simple parsing and the nested parsing. But it was not as straightforward as I thought it would be.

Anyway, will work on this soon.

@schveiguy

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2019

ping @s-ludwig I've fixed all the compilation issues (wow, it was bad. I should have tried testing before). Will monitor the CI to make sure everything works.

Also added unit tests.

@@ -1012,7 +1012,6 @@ final class HTTPServerRequest : HTTPRequest {

private void parseFormAndFiles() @safe {
_form = FormFields.init;
assert(!!bodyReader);

This comment has been minimized.

Copy link
@schveiguy

schveiguy Feb 13, 2019

Author Contributor

This is moved to parseFormData, because if the content type is not a form, bodyReader isn't used.

Makes unit testing much easier, since I don't have to invent a null input stream (which is suspiciously missing).

@schveiguy

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2019

The travis failure seems environment related. I don't understand the codecov report, I definitely am unit testing the code.

@s-ludwig

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

I restarted the failing travis job, looks like it should pass then. I think the CodeCov results are looking strange because the branch needs a rebase. It seems to contain all changes on master since (presumably) January.

Show resolved Hide resolved web/vibe/web/common.d Outdated
Show resolved Hide resolved web/vibe/web/common.d Outdated
@schveiguy

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2019

Also TODO: need to update docs.

schveiguy added some commits Jan 31, 2019

Rewrote array handling to handle both empty brackets and indexed ones…
…. Streamlined processing and removed some allocations.
Fix compilation issues.
Add rudimentary unit tests.

@schveiguy schveiguy force-pushed the schveiguy:addarrayhandling branch from ac26af9 to fb4db21 Feb 14, 2019

@schveiguy

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2019

OK, I fixed the issues I had identified. Should be good to go.

assert(result == ParamResult.ok);
assert(arr2 == [[2,4],[3],[1]]);

int[][2] staticarr2;

This comment has been minimized.

Copy link
@schveiguy

schveiguy Feb 14, 2019

Author Contributor

I modified this to a 2-element array to test that the arr[2][] field is ignored (found a bug in the process as well).

@schveiguy

This comment has been minimized.

Copy link
Contributor Author

commented Feb 16, 2019

ping @s-ludwig this should be all set.

@s-ludwig

This comment has been minimized.

Copy link
Member

commented Feb 17, 2019

Okay, looks good AFAICS. I'll merge and tag a beta release.

@s-ludwig s-ludwig merged commit 7f6660b into vibe-d:master Feb 17, 2019

3 checks passed

codecov/patch 0% of diff hit (target 35.008%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@schveiguy schveiguy deleted the schveiguy:addarrayhandling branch Feb 18, 2019

@schveiguy

This comment has been minimized.

Copy link
Contributor Author

commented Feb 18, 2019

TODO: need to update docs.

See #2266

@schveiguy

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

@s-ludwig I found a bug in this code (stupid operator precedence bug). Will submit a patch soon, don't release yet.

@schveiguy

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

Followup PR: #2270

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.