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

PyCrits bug - Campaign by Name #2

Open
tlansec opened this issue Jan 26, 2015 · 3 comments
Open

PyCrits bug - Campaign by Name #2

tlansec opened this issue Jan 26, 2015 · 3 comments

Comments

@tlansec
Copy link

tlansec commented Jan 26, 2015

Hi,

When executing the following code, the campaign is not retrieved successfully:

from pycrits import pycrits
crits = pycrits(uri,username,api_key)
params = { 'c-name' : 'somecampaign' }
print crits.campaign_by_name(params)

However, the following does allow you to retrieve it.

from pycrits import pycrits
crits = pycrits(uri,username,api_key)
params = { 'c-name' : '' }
print crits.campaign_by_name('somecampaign',params)

Not sure if this is by design, but given that an arg 'c-name' is parsed from campaign_by_name i would have expected it to be used in the query.

Cheers,
Tom

@wxsBSD
Copy link
Contributor

wxsBSD commented Jan 26, 2015

This is by design, though I'm open to suggestions on how to make it better.

This is the code for campaign_by_name():

def campaign_by_name(self, name, params={}):
        params['c-name'] = name
        results = self._single_fetch(self._CAMPAIGNS, params)
        return results['objects']

c-name is not parsed in campaign_by_name, it is set to whatever the required argument (name) is. In your first example the params dictionary passed to _single_fetch() looks like this:

{'c-name': {'c-name': 'somecampaign'}}

This is wrong for obvious reasons. The second example you give works, but the params option is unncessary as c-name is set in campaign_by_name. The correct way to call campaign_by_name if you have a name and no parameters is:

campaign_by_name('somecampaign')

If you have any parameters you want to pass along, like say only return the id, then you can do this:

campaign_by_name('somecampaign', {'only': 'id'})

@tlansec
Copy link
Author

tlansec commented Jan 26, 2015

Ah OK, got it. I had a feeling it was by design but couldn't figure it out by eye.

@wxsBSD
Copy link
Contributor

wxsBSD commented Jan 26, 2015

Yeah, I'm open to suggestions on how anyone thinks this should be done. Might as well provide a bit of background and use this issue to open up the discussion if anyone wants to chime in.

I designed the API to be as light-weight as possible. The idea was to make getting objects easy but it does require you to know what is and is not possible via the CRITs API. For example, to search for a sample via md5 you have to do this:

for sample in crits.samples({'c-md5': 'foo'}):
    print sample['filename']

You have to know that c-md5 is the right thing to use. This makes my code super lightweight because I don't have to validate any of these things, I just blindly pass them to CRITs and give the caller back the results in a reasonable fashion. The downside is that I'm pushing the knowledge requirement off to the consumer of my API.

I know I've had discussions with @wick2o about taking a different approach, which I'm certainly open to seeing a straw-man implementation of. His argument is essentially that we should mirror the CRITs object model and expose that. So querying for a specific sample by md5 becomes this (pseudo-code):

sample = pycrits.Sample()
sample.md5 = 'foo'
obj = sample.fetch()

The upside of this is that consumers of the API don't need to know about c-md5 or any of the CRITs API specific things. It makes it much easier to pick up pycrits and run with it. The downside is that pycrits becomes more difficult to write and maintain in the future as each new TLO in CRITs would need a corresponding object and all the plumbing to expose attributes and translate them into API calls, as opposed to just a handful of wrapper functions for a new TLO in the model I use now. Another thing I have not thought through with this idea is how to handle creating new objects.

I think one possibility might be to maintain both models? I'm certainly open to discussion here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants