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

[discuss] SOAP API: add method for searching within issues #560

Merged
merged 5 commits into from Feb 3, 2015

Conversation

mhabrnal
Copy link
Contributor

Add method 'mc_search_issues' to SOAP API. Filter is in JSON format.
It is also possible to filter by custom fields.

The example of filter with 'os' == "CentOS" and custom field 'abrt_hash' == "hash_content":

{"abrt_hash":"hash_content", "os":"CenstOS"}

Signed-off-by: Matej Habrnal mhabrnal@redhat.com

@mhabrnal mhabrnal changed the title SOAP API: add method for searching within issues [discuss] SOAP API: add method for searching within issues Dec 10, 2014
@vboctor
Copy link
Member

vboctor commented Dec 13, 2014

Thanks @mhabrnal for your contribution. There are few high level questions:

  • See Allow retrieving multiple/complex configs #528 for discussion about use of json. I haven't closed on this with @rombert
  • We will probably need a bit of documentation on expected json input in the developer manual if we go with json.
  • For custom fields, we typically prefix them with "custom_" when referencing them from a field list. So in your example, it would be "custom_abrt_hash".

<message name="mc_search_issuesRequest">
<part name="username" type="xsd:string" />
<part name="password" type="xsd:string" />
<part name="project_id" type="xsd:integer" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to support project_id here as a first class parameter and within the filter itself? In the advanced filter case we may have even multiple projects within the filter. We can keep this, but should document the behavior.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about making it project_ids and therefore and integer array?

@mhabrnal
Copy link
Contributor Author

Hi @vboctor, thank you for the quick response and good points for the patch.

  • project_id as a method parameter - I used the project_id as a method parameter because the other filtering methods in SOAP API used a similar scheme. In the case we will use this method doesn't matter if the project_id will be as a method parameter or as a part of the filter [1]. If it be better (for other users) to have possibility to search within all project (project_id will be part of the filter), i'll do it :)
  • custom_ prefix - ok
  • code formating - ok
  • JSON format of the filter - If I write a document with a few examples of how to create JSON filter, can the filter be in JSON format?

[1] https://github.com/abrt/libreport/wiki/MantisBT

@rombert
Copy link
Member

rombert commented Dec 16, 2014

@mhabrnal - thanks for the contribution! I'll settle on commenting on the high-level stuff, basically the WSDL, to make sure we agree on that first.

I actually started implementing this myself, but ran out of steam some time ago. You can see the status at that time at https://github.com/rombert/mantisbt/compare/soap-custom-filters . I don't think it can be ported as-is, but can serve as another reference.

@rombert
Copy link
Member

rombert commented Dec 16, 2014

@mhabrnal , @vboctor - I still think wrapping JSON in XML is not the way to go . I would prefer building a set of fields which are known to work and use that. Would we lose any functionality by going this route ?

@mhabrnal
Copy link
Contributor Author

To ensure proper functionality of ABRT MantisBT reporter we need to search only by 'Category', 'Project', 'OS Version' and custom field 'abrt_hash'. So we need a possibility to search by custom fields too. Anyway i think it's better to create searching method which can search by all possible parameters.

I liked the idea of JSON custom filters format because it's easy to implement it and it's easy to understand how it works and how to create it :)

@rombert - Thanks for inspiration :)

This version is without JSON format in filter. I have defined a new types
(FilterSearchData, IntegerArray, FilterCustomField and FilterCustomFieldArray).
Is possible to search within more then one project at the same time.

Signed-off-by: Matej Habrnal <mhabrnal@redhat.com>
@mhabrnal
Copy link
Contributor Author

Hi @vboctor, @rombert, The first version of the abrt mantis reporter is finished https://github.com/abrt/libreport/pull/313, so I had a time to work on the feedback from you and i have created a new version of the searching SOAP API method.

Matej Habrnal added 4 commits January 23, 2015 19:56
Signed-off-by: Matej Habrnal <mhabrnal@redhat.com>
mc_filter_search_issue_ids() - returns the array of issue ids.
mc_issues_get() - given an integer array returns array of issue data.
mc_issues_get_header() - given an integer array returns array of issue header data.

Fixes #8657

Signed-off-by: Matej Habrnal <mhabrnal@redhat.com>
Signed-off-by: Matej Habrnal <mhabrnal@redhat.com>
mc_filter_search_issue_ids()
mc_filter_search_issues()
mc_filter_search_issue_headers()
mc_issues_get()
mc_issues_get_header()

Signed-off-by: Matej Habrnal <mhabrnal@redhat.com>
<xsd:element name="category" type="tns:StringArray" minOccurs="0"/>
<xsd:element name="severity_id" type="tns:IntegerArray" minOccurs="0"/>
<xsd:element name="status_id" type="tns:IntegerArray" minOccurs="0"/>
<xsd:element name="priority_id" type="tns:StringArray" minOccurs="0"/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is priority_id a StringArray?

@vboctor
Copy link
Member

vboctor commented Jan 25, 2015

@mhabrnal this is so close. I've done a more thorough and think this is almost ready to go in. I would love to see this make it into the 1.3 release. So let's address these comments and get it in. It will be a great addition.

@mhabrnal mhabrnal force-pushed the master branch 3 times, most recently from a11034b to 764fad6 Compare January 25, 2015 22:29
@mhabrnal
Copy link
Contributor Author

@vboctor I made adjustments according to your comments. I hope it will be according to your suggestions.

@mhabrnal
Copy link
Contributor Author

@vboctor i added those three methods.

@@ -288,6 +330,19 @@
<xsd:element name="require_closed" type="xsd:boolean" minOccurs="0"/>
</xsd:all>
</xsd:complexType>
<xsd:complexType name="FilterCustomField">
<xsd:all>
<xsd:element name="field" type="tns:ObjectRef" minOccurs="0"/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this have minOccurs="1"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right.

@vboctor
Copy link
Member

vboctor commented Jan 26, 2015

It would be great if we can add some tests for these methods since we rely on such tests to validate the SOAP APIs.

@mhabrnal
Copy link
Contributor Author

ok, i'll try to write some tests tomorrow.

@mhabrnal
Copy link
Contributor Author

@vboctor i added the tests.

@vboctor
Copy link
Member

vboctor commented Jan 28, 2015

I think this is ready once the two minor comments are addressed. I will merge it right away then. Assuming TravisCI gives the thumbs up too :)

@mhabrnal mhabrnal force-pushed the master branch 3 times, most recently from aff6e01 to 8ae4b6a Compare January 29, 2015 10:36
@mhabrnal
Copy link
Contributor Author

@vboctor i addressed the comments. i hope it is right now.

@vboctor
Copy link
Member

vboctor commented Jan 29, 2015

@mhabrnal Did you revert to an older snapshot of the code somehow? I can only see 2 commits.

@mhabrnal
Copy link
Contributor Author

@vboctor i don't think so. I see 5 commits here.

@mhabrnal
Copy link
Contributor Author

@vboctor i reverted commits sometime. Mostly when the tests failed, but never to an older snapshot. On my branch are 5 commits too (see https://github.com/mhabrnal/mantisbt/commits/master).

<xsd:complexType name="FilterSearchData">
<xsd:all>
<xsd:element name="project_id" type="tns:IntegerArray" minOccurs="0"/>
<xsd:element name="search" type="xsd:string" minOccurs="0"/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want a more self-explanatory term than search? E.g. text_search, full_text_search or similar

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the web mantisbt filter UI is it named as search so i used the same name.

@rombert
Copy link
Member

rombert commented Jan 29, 2015

This looks great! I made a couple of cosmetic comments, but nothing which should block merging this PR in its current state. We can decide to address them in follow-up changes if needed.

Also tested generation of Java stubs from the new WSDL and it looks good.

@mhabrnal
Copy link
Contributor Author

mhabrnal commented Feb 2, 2015

@vboctor @rombert, please let me know, which of the comments i have to addressed. i'll send a patch ASAP :)

@rombert
Copy link
Member

rombert commented Feb 2, 2015

From my POV there is nothing to address for this PR to be merged

On Mon, Feb 2, 2015 at 3:22 PM, Matěj Habrnál notifications@github.com
wrote:

@vboctor https://github.com/vboctor @rombert
https://github.com/rombert, please let me know, which of the comments i
have to addressed. i'll send a patch ASAP :)


Reply to this email directly or view it on GitHub
#560 (comment).

http://robert.muntea.nu/

vboctor added a commit that referenced this pull request Feb 3, 2015
Fixes #8657: SOAP API support for custom filters
@vboctor vboctor merged commit 615f609 into mantisbt:master Feb 3, 2015
@vboctor
Copy link
Member

vboctor commented Feb 3, 2015

Thanks @mhabrnal for your contribution. It is great to have this feature implemented, we wanted to get to it for a while. Hopefully we work together further in the future.

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