axios-actions icon indicating copy to clipboard operation
axios-actions copied to clipboard

Allow placeholders which are not POSTed as data

Open Crote opened this issue 5 years ago • 18 comments

Currently, when requests are made with url placeholders, the placeholder values are always submitted to the server as part of the data. This is in many cases undesirable.

Usually, the server does not actually need those to be posted, as they can be retrieved from the url. In the best case, the server will just ignore them. In the worst case, the request will throw an error because the data contains unknown values. In addition to this, it makes it hard to change this value: for a template /foo/:name, if we want to submit a change to the name value, we can't use this template: the url requires the old value, but the data requires the new value, so we have to use a template like /foo/:_name, and we're once again adding data to our request which the server must ignore.

Ideally, the data and parameters would be two separate parameters, but that would break a lot of existing code. Perhaps the POST-like requests can have an optional second parameter, where it defaults to current behaviour when it is not set?

Crote avatar Feb 06 '19 17:02 Crote

Hey Laurens,

You've spotted Axios Action's dirty little secret!

Yes, everything gets submitted because generally it doesn't matter, and it's easier, but I've always suspected that at some point it may become a problem.

And you make a great point about changing a name.

Let's use this thread to think of solutions.

Another idea: a constructor option to remove the URL variable from the data payload.

davestewart avatar Feb 09 '19 15:02 davestewart

Actually, seeing as you specifically have this problem, can how do you find preparing the payload in the first place?

Do you feel you're forced into adding values to the payload that weren't there originally, just to satisfy the library's requirement to populate the path?

I'm just trying to think through the problem end to end.

davestewart avatar Feb 09 '19 15:02 davestewart

To be clear, this is not something which is currently a problem for my implementation. I just came across it and it seemed like something which could be a problem in the future.

The reason I noticed it, is because I tried to do something like posts.update(id, {title: 'new post', body: 'this is updated text'}), because it wasn't clearly documented how submitting data and placeholders interacted and that seemed the obvious way to do it considering the rest of the api. In my reasoning, it's not uncommon for the data and the url definition to come from completely different parts of your application, so putting them in one object would require merging them together when calling the api, so that doesn't really make sense.

Fortunately I'm using Ruby on Rails as backend, which happily ignores any parameter it does not recognize. so for me it's not a big deal at the moment.

However, consider something like the Gitlab Pages API, which has a post parameter which overlaps with a url placeholder. How would one implement that? Currently, the only option is to rename your placeholders client-side and hope the server is willing to ignore it.

Removing the placeholder before submitting would work if your server is very strict on the data it accepts, but it would still require the user to use a different name for their parameter. Note that this could also cause hard-to-find bugs if at some point in time the data submitted to the server gains an additional field, which just happens to overlap with an existing parameter.

Ideally, it should be impossible for the parameter name to overlap with a data key, but I'm not sure if that's even possible to do.

Perhaps defining symbols for the placeholders as properties on the api function would work, giving something like

Object.defineProperty(posts.update, 'id', { value: Symbol('id'), writable: false })
----
posts.update({posts.update.id: id, title: 'new post', body: 'this is updated text'})

It's not very elegant, but you can be sure that those don't overlap any payload data, so you can first look for those to fill in the parameters, falling back to current behaviour if they're not found. Unfortunately, the names can also overlap existing properties, so we get something like

posts.update({posts.update.param.id: id, title: 'new post', body: 'this is updated text'})

which is just way too verbose in my opinion.

Crote avatar Feb 09 '19 18:02 Crote

because it wasn't clearly documented how submitting data and placeholders interacted

Hmm. I should certainly make that clear if it isn't already.

OK, if this isn't a priority then I'll have a little think over the next short while and we can pick this up as and when

davestewart avatar Feb 09 '19 18:02 davestewart

Yep, we've just hit this same problem and due to the way we are using our APIDoc as server side validation we need the user ID removed from a call which would be like

const api = new APIResource(axios, './user/:userId/consent');
data = api.read(1); // returns {photo:true,public:false}
data.public = true;
api.update(data);

Would it not be possible to use the second param be used, such as?

const api = new APIResource(axios, './user/:userId/consent');
data = api.read(1); // returns {photo:true,public:false}
data.public = true;
api.update(data, {userId:1});

rlweb avatar May 22 '19 10:05 rlweb

Hey Rhys,

OK, looks like I'll have to do something about this sooner rather than later then!

So that took me a couple of reads, but you're saying that the return payload no longer has a parameter that will populate :userId so your idea is to pass additional path parameters as a second parameter.

It's been a while since I read this post, but I like it on the surface of things.

Is there no way you would do something like this, even with a helper?

api.update({...data, public: true, userId: 1});

davestewart avatar May 22 '19 12:05 davestewart

Hi Dave,

The issue is that our API has been built to ensure there is not additional unused parameters? So such as it see's userId and returns a validation exception!

rlweb avatar May 22 '19 17:05 rlweb

Hi @davestewart , I've attached a PR although its a little bit awkward with the setup of the id say within the read call. I've essentially allowed to be the last replacement during the replaceTokens function.

rlweb avatar May 22 '19 18:05 rlweb

Thanks for the PR.

Not sure if I'll accept it right off the bat as the only other outstanding issue is #14 which is for exactly the same issue.

There's a discussion in there.

Did you, or can you, take a look and feed back?

davestewart avatar May 22 '19 20:05 davestewart

Hey guys, "new project, new challenges" here.

@davestewart your plugin fits well on our need, but we got the same problem from @rlweb, and his fork worked like a charm.

stephandesouza avatar Oct 14 '19 20:10 stephandesouza

Hey hey, sorry for the lack of progress on this project - I have been mega-busy, but am now free again.

Can you pull from that fork in the meantime, and I will take a look in the next few days?

davestewart avatar Oct 14 '19 20:10 davestewart

Of course, if you need any hands for testing purposes, I'm here to help.

stephandesouza avatar Oct 14 '19 21:10 stephandesouza

Hello, spotted the same issue here as well, specifically for a child resource scenario.

I have a rest/groups/:groupId/members end point which you can use to add new members to a group by issuing a POST request. As per how axios-actions combines url parameters with body data, I end up sending the :groupId url param in the body of the POST request.

Any idea when this will be addressed?

vizio360 avatar Jan 04 '20 14:01 vizio360

Dont forget us @davestewart :)

stephandesouza avatar Feb 20 '20 19:02 stephandesouza

@davestewart Hello! People are still waiting!

AlMuz avatar Dec 21 '21 14:12 AlMuz

I had forgotten about this but... Christmas time is Open Source time! Maybe a 🎁 coming, that being the case...

davestewart avatar Dec 21 '21 14:12 davestewart

I haven't forgotten about you all! I'm fulfilling all my Open Source responsibilities by the end of Jan!

davestewart avatar Jan 10 '22 20:01 davestewart

Hey all...

Update

I have FINALLY had some time to look at this!

I'm actually on a new project where they need something like Axios Actions – but we're using TypeScript – so I just started playing with some raw code.

I haven't compared what I've written with AA's API, but the solution is pretty cool I think...

WIP

Take a look at this WIP (all in TypeScript):

  • https://stackblitz.com/edit/axios-actions-ts?devToolsHeight=100&file=src%2Flib.ts,src%2Fmain.ts

The key is a modified token syntax; append a ! to the token to signify "this is for the URL only":

PUT posts/:id!

If an object payload is passed, that key will be removed.

Also, I'm working on a modified method signature so you can pass multiple arguments:

posts.update(1, { title: 'Hello World' })

It supports:

  • as many token variables as required for the URL
  • an optional final Object payload

I've not fully compared the existing API yet.

Examples

Here's how you would set up read and update endpoints:

const users = makeEndpoints('https://jsonplaceholder.typicode.com', {
  search: 'users?:key=:value',
  read: 'users/:id',
  update: 'PUT users/:id!',
}, options)

And here's the new flexible way of calling them:

// pass multiple values, they will be used to replace tokens in order
users.search<User[]>('username', 'Bret').then(users => {
  console.log('search:', users.map(user => user))
})

// if you pass an object as the last parameter, it will be used as the payload
users.update<User>(5, { username: 'Steve' }).then(user => {
  console.log('update:', user)
})

// or, just pass the whole payload; tokens with a `-` will remove the value from the payload
users.update<User, User>({ id: 5, username: 'Steve' }).then(user => {
  console.log('update:', user)
})

Next steps

If this token syntax seems like a good solution (and you are still using the lib!) I will look to integrate it.

If you think this won't work, let me know and we can go from there.

I will also look to port AA to TS at some point too.

Sorry again for the huuuuuge delay! 🙏

davestewart avatar May 06 '22 05:05 davestewart