CenturyLinkCloud / clc-node-sdk

Apache License 2.0
6 stars 1 forks source link

Delete and Server Selection #196

Closed gjsmith66 closed 7 years ago

gjsmith66 commented 8 years ago

So... If you do this

var server; // server is undefined

serverService.delete({ name: server, dataCenter: [DataCenter.US_EAST_STERLING], groupId: 'ourgroupid' })

You will delete EVERY server in that Group. My guess is that if I did not have that groupId in there I would have deleted EVERY server that we have at CenturyLink.

Fun Stuff

push1st1k commented 8 years ago

Hi @gjsmith66! Let me explain a little bit how search criteria is processed.

If some criteria field is null, undefined, "", 0 - the data doesn't filtered by this field at all. Other words, this criteria is ignored. In this case search criteria will be transformed to

{
  dataCenter: [DataCenter.US_EAST_STERLING],
  groupId: 'ourgroupid'
}

The same behavior in #197

gjsmith66 commented 8 years ago

I understand, and saw in the code where this is happening. However...

1) This is behavior is not documented 2) I do not know of any other system that I have ever used where those values would result in essentially creating a wildcard that matches everything. If you did a Select in SQL and set a value to 0 or "", it does not match all. And, when you understand that you are indeed creating a wildcard, you see how dangerous this behavior is.
3) What is the most common Javascript error? That's right, passing an undefined variable. So, when testing (and remember CL has no Sandbox that I know of), you are creating the perfect storm for deleting all your servers... 4) I see no justification for handling this way. And if this is the behavior everywhere, are there cases where you ARE preventing a valid match on '' or 0? What if you wanted to find descriptions = ''? That would match all, not what someone would expect. I can't think of a field that you would want to search for a 0, but my guess is that there are some. And guess what? What happens is the exact opposite of what you would want.

My belief is that you should error on this condition, or treat the value literally and try to match on a '' or 0. A null or undefined are clearly cause to fail.

Please let me know if you have any questions.

Thanks!

push1st1k commented 8 years ago

@gjsmith66 Thank you for bringing our attention to this issue. Documentation will be updated soon. So now if you specify null or undefined as criteria property - it will search by this value, not omit it as was implemented before. I'm not sure that SDK should throw errors if user pass null.

Please update SDK to the latest one (1.1.4) and check, that all issues were fixed.

Regards, Alex