Autocompleter formids defaults to current form

assachs commented 5 months ago

Hello, after update to 5.0.4 (from 4.0.3) the formids attribute of autocompleter is set to the current form. Even if the attribute is not set. That leads to the situation, that all fields of the current form are added to the url (url: the url to fetch the content of the list; even if post is the request type). I have a large form and the application server is now complaining about too much parameters.

According to the documentation formids has no default value.

assachs commented 5 months ago

I tracked this down to this commit: https://github.com/struts-community-plugins/struts2-jquery/commit/9438d48dfed098dd3a44c2644d40275da1bfd19e

lukaszlenart commented 5 months ago

I assume you meant 5.0.3?

assachs commented 5 months ago

5.0.4-snapshot

lukaszlenart commented 4 months ago

@assachs could you check the PR?

assachs commented 4 months ago

Hello Lukasz, i did not try the new code, i just looked at the PR. And don't understand how this change should help. (please confirm if this should realy solve the issue #465)

According to https://github.com/struts-community-plugins/struts2-jquery/wiki/AnchorTag and this exmaple:

<s:form id="form" action="AjaxTest">
  <input type="textbox" name="data">   
</s:form>

<sj:a formId="form" targets="div1">Submit form</sj:a>

The sj:a is outside the form and with formId="form" (i think there is a typo in the example, it should be formIds) the textbox "data" is included in the link.

As far as i unterstand the PR, the change is only relevant if the sj:a is inside a form:


<s:form id="form" action="AjaxTest">
  <input type="textbox" name="data">   
  <sj:a targets="div1">Submit form</sj:a>
</s:form>

After the change the attribute formIds of sj:a is only automatically set, if the form has an id attribute. At the moment it is always set. Maybe this behaviour is good for the link (i'm not 100% sure: why should the a behave different depending on the id tag of the form. It does also not fit to the documentation: "Comma delimited list of form ids for which to serialize all fields during submission when this element is clicked. Default empty"), but i do not unterstand how this should work with autocompleter.

Let's say i have the following form:

<s:form ....>
    <sj:autocompleter href="url"  requestType="post" ..../>
    <s:textfield name="a" ..../>
</s:form>

I don't have a problem submitting the form. I have a problem when the autocompleter tries to fetch the entries. With the current version the value of textfield a is added to the url of the fetch request. As far as i unterstand the PR, the value will still be added, if the form has an id. But even in this situation the value should not be added as long as the attribute formids is not specified. My current workaround is to add formIds="non-existing-formid" to autocompleter.

I think your fix will solve the problem for about 90% of my forms, because they don't have an id. But it does not look like the real solution.

And there is another problem: the parameter for textfield a is added to the url (GET) even if the request type for the autocompleter is post.

Please correct me if I misunderstood the PR.

Andi

lukaszlenart commented 3 months ago

I don't understand the problem tbh :( I reverted changes related to AbstractFormElement introduced in #190 and just left the change related to Submit component.

gregh3269 commented 3 months ago

Maybe a producer in the showcase/page would make it easier to understand the issue.

assachs commented 3 months ago

You can already see the issue on the showcase page:

Go to widget -> Autocompleter with JSON Result

Type "ab" in the field "handle an array".

The Autocompleter tries to fetch languages with "ab".

With the old version a request is send to jsonlanguages.action with the querystring "term=ab". (The showcase is using autocompleter with get mode. With post mode term=ab is in the body).

With the current version all other fields of the form are also added to the querystring:

https://struts.jgeppert.com/struts2-jquery-showcase/jsonlanguages.action?echo=&echo_widget=ab&echo=&echo_widget=&echo=&echo_widget=&term=ab&_=1710146128701

(They are always added to the url. Even in the case of post mode)

With the current version the option "formids" of autocompleter is set to the current form:

options_languages_widget.formids = "formAutocompleteJson";

With the old version this was empty.

At the moment my workaround is to specify a formid that does not exist:

<sj:autocompleter formIds="nonExistingForm" ...>

According to the documentation the default value of the formIds attribute is empty.

gregh3269 commented 3 months ago

If I test 4.0.3 and check the generated js the formids are set.

jQuery(document).ready(function () {
    var options_languages_widget = {};
    options_languages_widget.hiddenid = "languages";
    options_languages_widget.delay = 50;
    options_languages_widget.minimum = 2;
    options_languages_widget.selectBox = false;
    options_languages_widget.forceValidOption = true;
    options_languages_widget.onfocustopics = "autocompleteFocus";
    options_languages_widget.onselecttopics = "autocompleteSelect";

    options_languages_widget.jqueryaction = "autocompleter";
    options_languages_widget.id = "languages_widget";
    options_languages_widget.name = "echo_widget";
    options_languages_widget.oncha = "autocompleteChange";
    options_languages_widget.onfocustopics = "autocompleteFocus";
    options_languages_widget.href = "/struts2-jquery-showcase/jsonlanguages.action";
    options_languages_widget.formids = "formAutocompleteJson";

    jQuery.struts2_jquery_ui.bind(jQuery('#languages_widget'),options_languages_widget);

 });
.....
jQuery(document).ready(function () { 
    var options_submitFormAutocompleteJson = {};
    options_submitFormAutocompleteJson.jqueryaction = "button";
    options_submitFormAutocompleteJson.id = "submitFormAutocompleteJson";
    options_submitFormAutocompleteJson.targets = "result";
    options_submitFormAutocompleteJson.href = "#";
    options_submitFormAutocompleteJson.formids = "formAutocompleteJson";
    options_submitFormAutocompleteJson.indicatorid = "indicator";
    options_submitFormAutocompleteJson.button = true;

    jQuery.struts2_jquery.bind(jQuery('#submitFormAutocompleteJson'),options_submitFormAutocompleteJson);

 });

and the submits match for search ab

http://tomcat403
/struts2-jquery-showcase/jsonlanguages.action?echo=&echo_widget=ab&echo=&echo_widget=&echo=&echo_widget=&term=ab&_=1710241527341

http://struts.jgeppert.com
/struts2-jquery-showcase/jsonlanguages.action?echo=&echo_widget=ab&echo=&echo_widget=&echo=&echo_widget=&term=ab&_=1710242574375

Looking through the commit history also, I could not find any mods to suggest they were ever blank. Is this what you are referring to?

assachs commented 3 months ago

Hello

i think there are several problems, and i mixed them.

if the form does not have an id: with 4.0.3 the fields are not added, with 5.0.3 they are added. The reason is the commit mentioned in the bug report (9438d48).
if the form has an id: (this is the showcase) the fields are added with 4.0.3 and 5.0.3. But is this the expected behaviour? according to the documentation:

autocompleter formIds: Comma delimited list of form ids for which to serialize all fields during submission when this element is clicked. Default: (no default)

I would expect that the fields are not added

Fields are added as query string: if the requestType=post the fields should be added to the body
(and a new problem) why does the showcase executes a get request? according to the documentation the default request type is post

I think Lukasz PR will solve (1) but the fields will still be added if thr form has an id (2)

gregh3269 commented 3 months ago

How are you generating the form without id's? If I remove them they are auto generated. v4.0.3

<s:form  action="echo" theme="xhtml">

<form id="echo" name="echo" action="/struts2-jquery-showcase/echo.action" method="post">

assachs commented 3 months ago

Yes, they are auto generated for the html code. But auto generated ids are not used for the java script for autocompleter with 4.0.3.

options_submitFormAutocompleteJson.formids = "";

And then the fields will not be added to the request.

gregh3269 commented 3 months ago

v4.0.3. OK sorry missed that, but don't you get an error on the form, as formids is missing?

We don't get a

options_submitFormAutocompleteJson.formids = "";

(and it does work as you want it to)

jQuery(document).ready(function () {
    var options_customersMap_widget = {};
    options_customersMap_widget.hiddenid = "customersMap";
    options_customersMap_widget.delay = 50;
    options_customersMap_widget.minimum = 2;
    options_customersMap_widget.selectBox = false;
    options_customersMap_widget.forceValidOption = false;
    options_customersMap_widget.onfocustopics = "autocompleteFocus";
    options_customersMap_widget.onselecttopics = "autocompleteSelect";

    options_customersMap_widget.list = "customersMap";

    options_customersMap_widget.jqueryaction = "autocompleter";
    options_customersMap_widget.id = "customersMap_widget";
    options_customersMap_widget.name = "echo_widget";
    options_customersMap_widget.oncha = "autocompleteChange";
    options_customersMap_widget.onfocustopics = "autocompleteFocus";
    options_customersMap_widget.href = "/struts2-jquery-showcase/jsoncustomers.action";

    jQuery.struts2_jquery_ui.bind(jQuery('#customersMap_widget'),options_customersMap_widget);

 });

jQuery(document).ready(function () { 
    var options_submitFormAutocompleteJson = {};
    options_submitFormAutocompleteJson.jqueryaction = "button";
    options_submitFormAutocompleteJson.id = "submitFormAutocompleteJson";
    options_submitFormAutocompleteJson.targets = "result";
    options_submitFormAutocompleteJson.href = "#";
    options_submitFormAutocompleteJson.indicatorid = "indicator";
    options_submitFormAutocompleteJson.button = true;

    jQuery.struts2_jquery.bind(jQuery('#submitFormAutocompleteJson'),options_submitFormAutocompleteJson);

 });

v5.0.3

jQuery(document).ready(function () {
    var options_languages_widget = {};
    options_languages_widget.hiddenid = "languages";
    options_languages_widget.delay = 50;
    options_languages_widget.minimum = 2;
    options_languages_widget.selectBox = false;
    options_languages_widget.forceValidOption = true;
    options_languages_widget.onfocustopics = "autocompleteFocus";
    options_languages_widget.onselecttopics = "autocompleteSelect";

    options_languages_widget.jqueryaction = "autocompleter";
    options_languages_widget.id = "languages_widget";
    options_languages_widget.name = "echo_widget";
    options_languages_widget.oncha = "autocompleteChange";
    options_languages_widget.onfocustopics = "autocompleteFocus";
    options_languages_widget.href = "/struts2-jquery-showcase/jsonlanguages.action";
    options_languages_widget.formids = "echo";

    jQuery.struts2_jquery_ui.bind(jQuery('#languages_widget'),options_languages_widget);
 });

jQuery(document).ready(function () {
    var options_submitFormAutocompleteJson = {};
    options_submitFormAutocompleteJson.jqueryaction = "button";
    options_submitFormAutocompleteJson.id = "submitFormAutocompleteJson";
    options_submitFormAutocompleteJson.targets = "result";
    options_submitFormAutocompleteJson.href = "#";
    options_submitFormAutocompleteJson.formids = "echo";
    options_submitFormAutocompleteJson.indicatorid = "indicator";
    options_submitFormAutocompleteJson.button = true;

    jQuery.struts2_jquery.bind(jQuery('#submitFormAutocompleteJson'),options_submitFormAutocompleteJson);

 });

assachs commented 3 months ago

I was writing from memory but i reverted back to 4.0.3 now (my own application, not the showcase).

options_submitFormAutocompleteJson.formids = "";

You are right. This line does not appear, it's missing.

But i do not get the error.

o.formids is undefined. But it does not throw an error. It's ignored.

...
self.addForms(o.formids, url)
...

addForms : function(forms, url) {
        var self = this;
        if (forms) {
            if (!self.loadAtOnce) {
                self.require("js/plugins/jquery.form" + self.minSuffix + ".js");
            }
            $.each(forms.split(','), function(i, f) {
                var q = $(self.escId(f)).formSerialize();
                url = self.addParam(url, q);
            });
        }
        return url;
    }

For me 4.0.3 was better. But 4.0.3 doesn't look consistent to me.

It shouldn't make any difference whether the id was generated manually or automatically (better with 5.0.4)
the fields of the current form should not be transmitted to autocompleter backend if attribute "formIds" is not specified (better with 4.0.3 but only with generated id)
the requestType should be taken into account (not ok with 4.0.3 and 5.0.4)
the default requestType for autocompleter is not according to the documentation (4.0.3 and 5.0.4)

gregh3269 commented 3 months ago

in com.jgeppert.struts2.jquery.components.AbstractFormElement

there was a change where I think it seems to use the parameters that will be set on the form, if I revert it back to 4.0.3 version it uses the one without the id, and it works as you want. I also do not get any js errors.

if (ancestor != null && StringUtils.isBlank(formIds)) {
       //addParameter(PARAM_FORM_IDS, ancestor.getParameters().get("id"));
       addParameter(PARAM_FORM_IDS, ((Form) ancestor).getId());
}

<s:form  action="echo" theme="xhtml">
/struts2-jquery-showcase/jsonlanguages.action?term=ab&_=1710339764471

<s:form id="formAutocompleteJson" action="echo" theme="xhtml">
/struts2-jquery-showcase/jsonlanguages.action?echo=&echo_widget=ab&echo=&echo_widget=&echo=&echo_widget=&term=ab&_=1710339843103

I will dig a bit more, but I don't know the history of the change. Can you test this on your system?

lukaszlenart commented 3 months ago

@gregh3269 I exactly did the same in my PR https://github.com/struts-community-plugins/struts2-jquery/pull/480

gregh3269 commented 3 months ago

OK great, does this patch work for @assachs?

I will test it myself also.

lukaszlenart commented 3 months ago

That's why I started this discussion 4 days ago :D

gregh3269 commented 3 months ago

Form works as needed, but does the submit need to be

addParameter(PARAM_FORM_IDS, form.getId());

rather than

addParameter(PARAM_FORM_IDS, form.getParameters().get("id"));

to match 4.0.3. It will have the wrong id?

gregh3269 commented 3 months ago

I think you are correct the Submit.java needs to be

addParameter(PARAM_FORM_IDS, form.getParameters().get("id"));

Testing the patch the submit does not work?

on 5.0.4, if I just change AbstractFormElement, the submit works OK (ish).

AbstractFormElement.java
if (ancestor != null && StringUtils.isBlank(formIds)) {
    addParameter(PARAM_FORM_IDS, ((Form) ancestor).getId());
}

Submit.java
if (form != null && StringUtils.isBlank(formIds)) {
    addParameter(PARAM_FORM_IDS, form.getParameters().get("id"));
}

assachs commented 3 months ago

Hello, first of all: Thank you very much for your work

For me the goal is not clear: do we want the 4.0.3 behavior?

If yes, than your PR will be fine.

But from my point of view, the 4.0.3 behavior is also not good. That is the point I wanted to discuss.

The behavior of 4.0.3 is not consistent: If you add an id to the form autocompleter adds all fields. If you do not add an id to the form, autocompleter will not add the fields. This shouldn't depend on the id of the form.
According to the documentation only the fields of the forms specified with attribute "formIds" are added to the request. Is it expected that the fields of the current form will be added if formIds is not specified? (i think for a submit button this is good. But for a request to the autocompleter backend?)
The parameters are always added to the url. This should depend on the request type. If request-type = POST the parameters should be added to the body.
According to the documentation the default requestType of autocompleter is POST. In reality it is GET. I think the documentation should be updated (to avoid breaking things)

Maybe the following is not clear: "id of the form" = the attribute "id" of the "form" element "formIds" = the attribute "formIds" of the "autocompleter" element

gregh3269 commented 3 months ago

1 & 2. Possibly submit all the fields on the form to keep things simple.

3 & 4. There are lots of options on the tag, have you explored these?

requestType == Type of the AJAX Request. POST, GET, PUT

Alas, I could not find one for the actual submit, to control locally what gets submitted.

assachs commented 3 months ago

1&2. I am with you for a submit button. But not for autocompleter. Let's say, we have a form to create a user. Fields: name, given name, telephone, email and country. Country is a autocompleter. Why should all the fields be passed to the autocompleter backend to fetch the country list? According to the documentation formIds is empty. My interpretation was that only the input in autocompleter would be transferred (this corresponds to the 4.0.3 behavior without an id in the form) and not the complete form. If I want everything to be transferred, then I set formIds to the current form. With the current implementation, I have to set formIds to an id of a form that doesn't exist to prevent the submission.

If i specify requestType=POST then term=... is moved to the body, but all the other fields stay in the URL. I did not find another option for that (not in the documentation and not in the source)
the showcase has no requestType. According to https://github.com/struts-community-plugins/struts2-jquery/wiki/AutocompleterTag the default is POST. But the showcase is doing a GET. This could be fixed without breaking an application by changing the documentation.

gregh3269 commented 3 months ago

Your country list is pretty simple use case, if you are writing a configurator with many dependant options, you want the previous selections. The easiest way would be to just grab the form and serialise it, which is I guess what happens here. After all the parameters are cheap.

I did not look into the error with the o.formids on 4.0.3 (above). It possibly could be a bug resulting in only sending the single parameter, which you want. Bugs as "features" are risky to use and the error should have been addressed earlier.

The AutocompleterTag does have a requestType?

Yes, the docs could do with being updated.

We have to see what @lukaszlenart decides to do here, as its not really a bug. Possibly for simplicity sending the form data is OK regardless on the formid.

assachs commented 3 months ago

Bugs as "features" are risky to use and the error should have been addressed earlier. That's the reason why i said: don't bring back the 4.0.3 behavior because it's inconsistent.

I started this discussion to clarify the expected behavior and to find out if this is a bug or the expected behavior.

Your country list is pretty simple use case, if you are writing a configurator with many dependant options, you want the previous selections. The easiest way would be to just grab the form and serialise it, which is I guess what happens here. After all the parameters are cheap.

I also thought about this usecase. I think there are two solutions:

Keep it as it is with 5.0.3. Extend the documentation that formIds defaults to the current form. And maybe add an attribute to disable sending of the parameters (this won't break anything)
Only send the data if formIds is specified. This will break applications if they don't specify the formId and depend on the old behavior.

Do you agree that the fields should also respect the requestType? at the moment they are added to the url even if the request type is POST

My problem with the current implementation: all fields are added to the url and i get an "414 Request-URI Too Long" error. To stop this i have to add formIds="nonexistingform".

If the requestType is taken into account, then it is no longer so important that you can switch it off.

lukaszlenart commented 3 months ago

Can we address one issue and then try to resolve the another one?

assachs commented 3 months ago

Can we address one issue and then try to resolve the another one?

Sure. We can close this ticket, if it's the expected behavior that all fields of the current form are send to the autocompleter backend if formIds is not specified.

lukaszlenart commented 3 months ago

Does #480 address one of the issues?

assachs commented 3 months ago

I am not sure what #480 does. Does #480 mean: only set formIds of autocompleter to the current form if id of the current form is specified and not generated?

if (((Form) ancestor).getId() != null) Is it null, wenn id is not specified? And form.getParameters().get("id") is never null, because a generated id will be returned?

If yes:

I think, #480 will bring back the 4.0.3 behavior. But since the expected behavior is that all fields of the form are submitted, this PR brings back a not wanted behavior. #480 does not address one of the issues.

When I opened the ticket, I thought that a bug was introduced from 4.0.3 to 5.0.4. In fact, this is the expected behavior and 3 other errors/inaccuracies were discovered instead:

default of formIDs is not empty. It's the current form. (Update documentation.)
Documentation of default requestType is wrong (POST -> GET)
Fields should be added to body instead of URL if requestType is POST But as you said before, this should be solved afterwards.

lukaszlenart commented 3 weeks ago

I want to finally circle back to this and release a new version. So the plan is to merge #480 and then address the three bullet points above. If not objections I will start working on that.

lukaszlenart commented 2 weeks ago

About POST vs GET - the default for $.ajax() (see type) is GET - the doc is wrong, either I can fix the doc or use POST as default (I can register a dedicated issue to better explain this problem).

lukaszlenart commented 2 weeks ago

Fixed default to GET in #522

lukaszlenart commented 2 weeks ago

Also updated docs

struts-community-plugins / struts2-jquery

Autocompleter formids defaults to current form #465