Tried making a Location w/ a Data Converter for customer... I would have expected it to work? Otherw...

# ktor

[dave08]

Tried making a Location w/ a Data Converter for customer... I would have expected it to work? Otherwise I'd have a Location with

data class UserRequest(val `customer[id]`: Int ...)

... 🙈

[Deactivated User]

Did you get this working? Your suggestion was about having an annotation or something to specify a different name from the property itself?

[dave08]

We ended up trying like this:

call.parameters
           .flattenEntries()
           .filterNot { it.first == "id" }
           .map { it.first.removeSurrounding("customer[", "]") to it.second }
           .toMap()

And then manually creating the customer object w/o any Location... 😕

[Deactivated User]

Aha, and you would be able to use it with an annotation like @Name("customer[id]") val customerId: String?

[dave08]

Nope, usually I would expect it to work like forms... data class UserRequest(val requestId: Int, customer: Customer)... that might be a bit hard w/ parameters defined as Map<String, List<String>> though...

[Deactivated User]

How do you create a form that group some properties like that? Or is it just convention for the name attributes?

[Deactivated User]

I guess it is convention? https://stackoverflow.com/questions/5802057/how-can-i-group-form-elements

[dave08]

In php, it's not just a convention, it actually creates arrays/maps based on this...

[dave08]

https://mattstauffer.com/blog/a-little-trick-for-grouping-fields-in-an-html-form/

[Deactivated User]

yeah I know, but it is not in the spec, or something like:
<group name=“customer”>
<input name=“name”>

[Deactivated User]

I have used PHP for a lot of years and I know how it works, but it is only for PHP or for frameworks supporting that syntax, not a spec from the w3c

[Deactivated User]

still I agree it is convenient

[dave08]

Actually: https://stackoverflow.com/questions/4295782/how-do-you-extract-post-data-in-node-js

[Deactivated User]

yeah, but that’s express; a framework that decided to support it

[dave08]

It looks like Express puts it in the body...?

[Deactivated User]

uhm

[dave08]

Not so clean... but an idea! In the end, I would maybe have thought of it for Locations and DataConverter to do the work... not to do such magic behind the user's back!

[Deactivated User]

I would create an issue with a feature request, linking to the resources in this thread, if you have managed to create a DataConverter doing that, maybe even a PR would be nice 🙂

[Deactivated User]

would be nice to limit it somehow to prevent degenerate cases (hash collision attacks or memory flooding)

[dave08]

Btw I already created a pull request for metrics (in my own repo) and referred the issue to it.. who goes over PRs?

[dave08]

It still needs to work out little details I mentioned in the PR though...

[Deactivated User]

Yeah, I have seen that and looks fine to me (I’m not the one reviewing PRs). Usually Sergey, but that usually happen in batches. Think about holidays and other projects like kotlinx-io. I have also two pending PRs for more than two weeks.

To be able to use earlier, I would suggest you to build locally and publish your changes to your own artifactory/ or bintray in the meantime.

Also I would make a proper PR the the repo as long as all tests are passing.

[dave08]

Meaning pressing the Merge button?

[dave08]

I don't have a chance to do too many PRs... the boss keeps us on our toes...

[Deactivated User]

I mean that the PR you did was in your own repository instead of the ktor one

[Deactivated User]

which reduces its visibility

[dave08]

Right, I thought that that's what has to be done.. make a fork, commit changes, and start a pull request... but after all that, there's a merge button that I didn't dare press (which I think is what actually puts the pull request in Ktor's repo... but Merge is a pretty suspicious name...)

[Deactivated User]

you have the merge button because it was your own repo

[Deactivated User]

and you have permissions there

[dave08]

So I just go to Ktor and create the pull request straight on the main repo?

[Deactivated User]

I guess when you create the PR you can choose the repo where you want to make it, by default it is the original repo AFAIK

[Deactivated User]

https://github.com/dave08/ktor/tree/dave08-implement-micrometer-metrics

[Deactivated User]

usually in a branch, there is a button to create a PR

[Deactivated User]

now that htere is one PR it doesn’t appear

[Deactivated User]

but I guess you can delete your PR in your repo and create a new one

[Deactivated User]

https://github.com/ktorio/ktor/compare/master...dave08:dave08-implement-micrometer-metrics

[Deactivated User]

here is another Create pull request button

[Deactivated User]

if you press the Create pull request button

[Deactivated User]

you will see at the header 4 dropdowns

[Deactivated User]

and a left arrow

[Deactivated User]

that’s the direction

[Deactivated User]

base fork: ktorio/ktor base: master <- head fork: dave08/ktor compare: dave08-implement-micro

[Deactivated User]

and that’s fine as it is

[Deactivated User]

I guess you changed it

[dave08]

So I just press that button and add the remarks to that PR?

[Deactivated User]

yes

[dave08]

Done! Thanks for all the help simple smile