You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by kufd <gi...@git.apache.org> on 2017/03/19 18:23:42 UTC
[GitHub] thrift pull request #1215: Thrift 4126
GitHub user kufd opened a pull request:
https://github.com/apache/thrift/pull/1215
Thrift 4126
Hello
It is a pull request with required fields validation for php extension
Here is jira issue https://issues.apache.org/jira/browse/THRIFT-4126
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/kufd/thrift THRIFT-4126
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/1215.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1215
----
commit 5f5df5f790508f9b8400d3c92d80c16b38546c18
Author: myroslav.kosinskyi <my...@skelia.com.ua>
Date: 2016-05-23T08:16:12Z
fix performance issue when try to deserialize big message with small read buffer size
commit 2bfce820efd6aa47ee12d91ff08fe310a753049a
Author: kufd <ko...@ukr.net>
Date: 2016-06-16T18:41:22Z
Merge remote-tracking branch 'upstream/master'
commit 13231f8190583ddcca410a4a3f961628754cbf5e
Author: kufd <ko...@ukr.net>
Date: 2016-07-23T09:56:02Z
Merge remote-tracking branch 'upstream/master'
commit f07e38cc911e01846c85480de4bd5eb2bc365092
Author: kufd <ko...@ukr.net>
Date: 2017-03-13T17:17:34Z
merge with upstream master
commit 9312aa8930bc26a100c46153f47c4860b1169753
Author: kufd <ko...@ukr.net>
Date: 2017-03-19T17:48:50Z
implement required fields validation in php extension
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1215: Thrift 4126
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1215
Yes, I recommend you:
1. Squash your commits (optional, but we'll do it anyway when we merge into apache master)
2. Rebase against upstream master.
3. Force push.
This will trigger a new build against the tip.
If you are not familiar with squashing, it's okay if you skip that part.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1215: Thrift 4126
Posted by kufd <gi...@git.apache.org>.
Github user kufd commented on the issue:
https://github.com/apache/thrift/pull/1215
Yes
I will add functional tests
But it can take a few weeks, because i do not have enough free time now
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1215: Thrift 4126
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1215#discussion_r108155957
--- Diff: lib/php/src/ext/thrift_protocol/php_thrift_protocol.cpp ---
@@ -728,6 +728,42 @@ inline bool ttypes_are_compatible(int8_t t1, int8_t t2) {
return ((t1 == t2) || (ttype_is_int(t1) && ttype_is_int(t2)));
}
+//is used to validate objects before serialization and after deserialization. For now, only required fields are validated.
+static
+void validate_thrift_object(zval* object) {
+
+ HashPosition key_ptr;
+ zval** val_ptr;
+
+ TSRMLS_FETCH();
+ zend_class_entry* object_class_entry = zend_get_class_entry(object TSRMLS_CC);
+ HashTable* spec = Z_ARRVAL_P(zend_read_static_property(object_class_entry, "_TSPEC", 6, false TSRMLS_CC));
+
+ for (zend_hash_internal_pointer_reset_ex(spec, &key_ptr); zend_hash_get_current_data_ex(spec, (void**)&val_ptr, &key_ptr) == SUCCESS; zend_hash_move_forward_ex(spec, &key_ptr)) {
+ ulong fieldno;
+ if (zend_hash_get_current_key_ex(spec, NULL, NULL, &fieldno, 0, &key_ptr) != HASH_KEY_IS_LONG) {
+ throw_tprotocolexception("Bad keytype in TSPEC (expected 'long')", INVALID_DATA);
+ return;
+ }
+ HashTable* fieldspec = Z_ARRVAL_PP(val_ptr);
+
+ // field name
+ zend_hash_find(fieldspec, "var", 4, (void**)&val_ptr);
+ char* varname = Z_STRVAL_PP(val_ptr);
+
+ zend_hash_find(fieldspec, "isRequired", 11, (void**)&val_ptr);
+ bool is_required = Z_BVAL_PP(val_ptr);
+
+ zval* prop = zend_read_property(object_class_entry, object, varname, strlen(varname), false TSRMLS_CC);
+
+ if (is_required && Z_TYPE_P(prop) == IS_NULL) {
+ char errbuf[128];
+ snprintf(errbuf, 128, "Required field %s.%s is unset!", object_class_entry->name, varname);
+ throw_tprotocolexception(errbuf, INVALID_DATA);
--- End diff --
I wonder if folks think it would be useful to add a new TProtocolException cause for this case, MISSING_REQUIRED, across all languages. While technically it is invalid data, I usually think of that as something got garbled.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1215: Thrift 4126
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1215
When this does merge into master, it's probably worth updating the README.md indicating this is a breaking change (for the better). I would like to take a look at how some of the other runtime libraries for Thrift handle required fields as well to see what the consensus behavior is.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1215: Thrift 4126
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/thrift/pull/1215
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1215: Thrift 4126
Posted by kufd <gi...@git.apache.org>.
Github user kufd commented on the issue:
https://github.com/apache/thrift/pull/1215
There is a falg "validate" in thrift generator for php (validate: Generate PHP validator methods\n")
And that flag is used in my changes
I will try to add tests for both cases: when the flag is set and when not
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1215: Thrift 4126
Posted by kufd <gi...@git.apache.org>.
Github user kufd commented on the issue:
https://github.com/apache/thrift/pull/1215
Now all build jobs are success
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1215: Thrift 4126
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1215
I'll open a story in Jira for it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1215: Thrift 4126
Posted by kufd <gi...@git.apache.org>.
Github user kufd commented on the issue:
https://github.com/apache/thrift/pull/1215
Hi @jeking3
I looked into the failed builds. But errors are not related to my changes.
Maybe i should wait for some time and merge changes from thrift repo into my branch.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---